Re: [PATCH v8 1/2] crypto: skcipher AF_ALG - overhaul memory management

[Stephan Müller]

Am Samstag, 10. Juni 2017, 10:59:40 CEST schrieb Herbert Xu:

Hi Herbert,

> 
> Indeed, we should just kill the wait or perhaps convert it to
> a WARN_ON.

As the inflight variable is only used for refcounting and the related wait, I 
would propose to remove that variable entirely.

Ciao
Stephan

Re: [PATCH v8 1/2] crypto: skcipher AF_ALG - overhaul memory management

[Herbert Xu]

On Sat, Jun 10, 2017 at 10:33:28AM +0200, Stephan Müller wrote:
>
> Right. Shouldn't we drop the ctx->inflight completely?
> 
> The code in the current patch set contains:
> 
> when an async operation is queued:
> 
> sock_hold(sk);
> ctx->inflight++;
> 
> upon completion of the callback:
> 
> __sock_put(sk);
> ctx->inflight--;
> 
> Thus, the socket is grabbed already. Hence, when dropping the inflight code 
> including the wait queue entirely, I would think we are still save as we hold 
> the socket.

Indeed, we should just kill the wait or perhaps convert it to
a WARN_ON.

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v8 1/2] crypto: skcipher AF_ALG - overhaul memory management

[Stephan Müller]

Am Samstag, 10. Juni 2017, 05:13:16 CEST schrieb Herbert Xu:

Hi Herbert,

> On Tue, May 23, 2017 at 04:31:59PM +0200, Stephan Müller wrote:
> >  static void skcipher_sock_destruct(struct sock *sk)
> >  {
> >  
> > struct alg_sock *ask = alg_sk(sk);
> > struct skcipher_ctx *ctx = ask->private;
> > 
> > -   struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(>req);
> > +   struct sock *psk = ask->parent;
> > +   struct alg_sock *pask = alg_sk(psk);
> > +   struct skcipher_tfm *skc = pask->private;
> > +   struct crypto_skcipher *tfm = skc->skcipher;
> > 
> > -   if (atomic_read(>inflight))
> > -   skcipher_wait(sk);
> > +   /* Suspend caller if AIO operations are in flight. */
> > +   wait_event_interruptible(skcipher_aio_finish_wait,
> > +(ctx->inflight == 0));
> 
> This doesn't look right.  If a signal comes in wouldn't you end
> up freeing live memory?

Right. Shouldn't we drop the ctx->inflight completely?

The code in the current patch set contains:

when an async operation is queued:

sock_hold(sk);
ctx->inflight++;

upon completion of the callback:

__sock_put(sk);
ctx->inflight--;

Thus, the socket is grabbed already. Hence, when dropping the inflight code 
including the wait queue entirely, I would think we are still save as we hold 
the socket.

Ciao
Stephan

Re: [PATCH v8 1/2] crypto: skcipher AF_ALG - overhaul memory management

[Herbert Xu]

On Tue, May 23, 2017 at 04:31:59PM +0200, Stephan Müller wrote:
>
>  static void skcipher_sock_destruct(struct sock *sk)
>  {
>   struct alg_sock *ask = alg_sk(sk);
>   struct skcipher_ctx *ctx = ask->private;
> - struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(>req);
> + struct sock *psk = ask->parent;
> + struct alg_sock *pask = alg_sk(psk);
> + struct skcipher_tfm *skc = pask->private;
> + struct crypto_skcipher *tfm = skc->skcipher;
>  
> - if (atomic_read(>inflight))
> - skcipher_wait(sk);
> + /* Suspend caller if AIO operations are in flight. */
> + wait_event_interruptible(skcipher_aio_finish_wait,
> +  (ctx->inflight == 0));

This doesn't look right.  If a signal comes in wouldn't you end
up freeing live memory?

The existing code is crap too.  We should not be waiting in the
destructor in the first place.  The proper way to do it is to hold
a reference count on the socket.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt