| Message ID | 20171218161045.0000161c@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
Sorry idiot moment. Sent that out with our internal markings. Resending shortly. On Tue, 19 Dec 2017 10:27:24 +0000 Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > This variable was increased and decreased without any protection. > Result was an occasional misscount and negative wrap around resulting > in false resource allocation failures. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code") > --- > The fixes tag is probably not ideal as I'm not 100% sure when this actually > became a bug. The code in question was introduced in > > Fixes: e870456d8e7c ("crypto: algif_skcipher - overhaul memory management") > and related. > rcvused moved in > Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code") > So I have gone with the latter option as that is where it will cleanly apply. > However, it probably doesn't matter as both are present only in the 4.14 > final kernel. > > crypto/af_alg.c | 4 ++-- > crypto/algif_aead.c | 2 +- > crypto/algif_skcipher.c | 2 +- > include/crypto/if_alg.h | 5 +++-- > 4 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/crypto/af_alg.c b/crypto/af_alg.c > index 8612aa36a3ef..759cfa678c04 100644 > --- a/crypto/af_alg.c > +++ b/crypto/af_alg.c > @@ -664,7 +664,7 @@ void af_alg_free_areq_sgls(struct af_alg_async_req *areq) > unsigned int i; > > list_for_each_entry_safe(rsgl, tmp, &areq->rsgl_list, list) { > - ctx->rcvused -= rsgl->sg_num_bytes; > + atomic_sub(rsgl->sg_num_bytes, &ctx->rcvused); > af_alg_free_sg(&rsgl->sgl); > list_del(&rsgl->list); > if (rsgl != &areq->first_rsgl) > @@ -1173,7 +1173,7 @@ int af_alg_get_rsgl(struct sock *sk, struct msghdr *msg, int flags, > > areq->last_rsgl = rsgl; > len += err; > - ctx->rcvused += err; > + atomic_add(err, &ctx->rcvused); > rsgl->sg_num_bytes = err; > iov_iter_advance(&msg->msg_iter, err); > } > diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c > index db9378a45296..d3da3b0869f5 100644 > --- a/crypto/algif_aead.c > +++ b/crypto/algif_aead.c > @@ -550,7 +550,7 @@ static int aead_accept_parent_nokey(void *private, struct sock *sk) > INIT_LIST_HEAD(&ctx->tsgl_list); > ctx->len = len; > ctx->used = 0; > - ctx->rcvused = 0; > + atomic_set(&ctx->rcvused, 0); > ctx->more = 0; > ctx->merge = 0; > ctx->enc = 0; > diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c > index c7c75ef41952..a5b4898f625a 100644 > --- a/crypto/algif_skcipher.c > +++ b/crypto/algif_skcipher.c > @@ -388,7 +388,7 @@ static int skcipher_accept_parent_nokey(void *private, struct sock *sk) > INIT_LIST_HEAD(&ctx->tsgl_list); > ctx->len = len; > ctx->used = 0; > - ctx->rcvused = 0; > + atomic_set(&ctx->rcvused, 0); > ctx->more = 0; > ctx->merge = 0; > ctx->enc = 0; > diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h > index 38d9c5861ed8..f38227a78eae 100644 > --- a/include/crypto/if_alg.h > +++ b/include/crypto/if_alg.h > @@ -18,6 +18,7 @@ > #include <linux/if_alg.h> > #include <linux/scatterlist.h> > #include <linux/types.h> > +#include <linux/atomic.h> > #include <net/sock.h> > > #include <crypto/aead.h> > @@ -150,7 +151,7 @@ struct af_alg_ctx { > struct crypto_wait wait; > > size_t used; > - size_t rcvused; > + atomic_t rcvused; > > bool more; > bool merge; > @@ -215,7 +216,7 @@ static inline int af_alg_rcvbuf(struct sock *sk) > struct af_alg_ctx *ctx = ask->private; > > return max_t(int, max_t(int, sk->sk_rcvbuf & PAGE_MASK, PAGE_SIZE) - > - ctx->rcvused, 0); > + atomic_read(&ctx->rcvused), 0); > } > > /**
Am Dienstag, 19. Dezember 2017, 11:31:22 CET schrieb Jonathan Cameron: Hi Jonathan, > This variable was increased and decreased without any protection. > Result was an occasional misscount and negative wrap around resulting > in false resource allocation failures. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code") > --- > Resending due to incompetence on my part - sorry all! > > The fixes tag is probably not ideal as I'm not 100% sure when this actually > became a bug. The code in question was introduced in > > Fixes: e870456d8e7c ("crypto: algif_skcipher - overhaul memory management") > and related. > rcvused moved in > Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code") > So I have gone with the latter option as that is where it will cleanly > apply. However, it probably doesn't matter as both are present only in the > 4.14 final kernel. > > crypto/af_alg.c | 4 ++-- > crypto/algif_aead.c | 2 +- > crypto/algif_skcipher.c | 2 +- > include/crypto/if_alg.h | 5 +++-- > 4 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/crypto/af_alg.c b/crypto/af_alg.c > index 8612aa36a3ef..759cfa678c04 100644 > --- a/crypto/af_alg.c > +++ b/crypto/af_alg.c > @@ -664,7 +664,7 @@ void af_alg_free_areq_sgls(struct af_alg_async_req > *areq) unsigned int i; > > list_for_each_entry_safe(rsgl, tmp, &areq->rsgl_list, list) { > - ctx->rcvused -= rsgl->sg_num_bytes; > + atomic_sub(rsgl->sg_num_bytes, &ctx->rcvused); > af_alg_free_sg(&rsgl->sgl); > list_del(&rsgl->list); > if (rsgl != &areq->first_rsgl) > @@ -1173,7 +1173,7 @@ int af_alg_get_rsgl(struct sock *sk, struct msghdr > *msg, int flags, > > areq->last_rsgl = rsgl; > len += err; > - ctx->rcvused += err; > + atomic_add(err, &ctx->rcvused); > rsgl->sg_num_bytes = err; > iov_iter_advance(&msg->msg_iter, err); > } > diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c > index db9378a45296..d3da3b0869f5 100644 > --- a/crypto/algif_aead.c > +++ b/crypto/algif_aead.c > @@ -550,7 +550,7 @@ static int aead_accept_parent_nokey(void *private, > struct sock *sk) INIT_LIST_HEAD(&ctx->tsgl_list); > ctx->len = len; > ctx->used = 0; > - ctx->rcvused = 0; > + atomic_set(&ctx->rcvused, 0); I think ATOMIC_INIT(0) is more suitable here. > ctx->more = 0; > ctx->merge = 0; > ctx->enc = 0; > diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c > index c7c75ef41952..a5b4898f625a 100644 > --- a/crypto/algif_skcipher.c > +++ b/crypto/algif_skcipher.c > @@ -388,7 +388,7 @@ static int skcipher_accept_parent_nokey(void *private, > struct sock *sk) INIT_LIST_HEAD(&ctx->tsgl_list); > ctx->len = len; > ctx->used = 0; > - ctx->rcvused = 0; > + atomic_set(&ctx->rcvused, 0); dto. > ctx->more = 0; > ctx->merge = 0; > ctx->enc = 0; > diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h > index 38d9c5861ed8..f38227a78eae 100644 > --- a/include/crypto/if_alg.h > +++ b/include/crypto/if_alg.h > @@ -18,6 +18,7 @@ > #include <linux/if_alg.h> > #include <linux/scatterlist.h> > #include <linux/types.h> > +#include <linux/atomic.h> > #include <net/sock.h> > > #include <crypto/aead.h> > @@ -150,7 +151,7 @@ struct af_alg_ctx { > struct crypto_wait wait; > > size_t used; > - size_t rcvused; > + atomic_t rcvused; > > bool more; > bool merge; > @@ -215,7 +216,7 @@ static inline int af_alg_rcvbuf(struct sock *sk) > struct af_alg_ctx *ctx = ask->private; > > return max_t(int, max_t(int, sk->sk_rcvbuf & PAGE_MASK, PAGE_SIZE) - > - ctx->rcvused, 0); > + atomic_read(&ctx->rcvused), 0); > } > > /** Other than the comments above: Reviewed-by: Stephan Mueller <smueller@chronox.de> Ciao Stephan
On Tue, 19 Dec 2017 12:11:19 +0100 Stephan Mueller <smueller@chronox.de> wrote: > Am Dienstag, 19. Dezember 2017, 11:31:22 CET schrieb Jonathan Cameron: > > Hi Jonathan, > > > This variable was increased and decreased without any protection. > > Result was an occasional misscount and negative wrap around resulting > > in false resource allocation failures. > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code") > > --- > > Resending due to incompetence on my part - sorry all! > > > > The fixes tag is probably not ideal as I'm not 100% sure when this actually > > became a bug. The code in question was introduced in > > > > Fixes: e870456d8e7c ("crypto: algif_skcipher - overhaul memory management") > > and related. > > rcvused moved in > > Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code") > > So I have gone with the latter option as that is where it will cleanly > > apply. However, it probably doesn't matter as both are present only in the > > 4.14 final kernel. > > > > crypto/af_alg.c | 4 ++-- > > crypto/algif_aead.c | 2 +- > > crypto/algif_skcipher.c | 2 +- > > include/crypto/if_alg.h | 5 +++-- > > 4 files changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/crypto/af_alg.c b/crypto/af_alg.c > > index 8612aa36a3ef..759cfa678c04 100644 > > --- a/crypto/af_alg.c > > +++ b/crypto/af_alg.c > > @@ -664,7 +664,7 @@ void af_alg_free_areq_sgls(struct af_alg_async_req > > *areq) unsigned int i; > > > > list_for_each_entry_safe(rsgl, tmp, &areq->rsgl_list, list) { > > - ctx->rcvused -= rsgl->sg_num_bytes; > > + atomic_sub(rsgl->sg_num_bytes, &ctx->rcvused); > > af_alg_free_sg(&rsgl->sgl); > > list_del(&rsgl->list); > > if (rsgl != &areq->first_rsgl) > > @@ -1173,7 +1173,7 @@ int af_alg_get_rsgl(struct sock *sk, struct msghdr > > *msg, int flags, > > > > areq->last_rsgl = rsgl; > > len += err; > > - ctx->rcvused += err; > > + atomic_add(err, &ctx->rcvused); > > rsgl->sg_num_bytes = err; > > iov_iter_advance(&msg->msg_iter, err); > > } > > diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c > > index db9378a45296..d3da3b0869f5 100644 > > --- a/crypto/algif_aead.c > > +++ b/crypto/algif_aead.c > > @@ -550,7 +550,7 @@ static int aead_accept_parent_nokey(void *private, > > struct sock *sk) INIT_LIST_HEAD(&ctx->tsgl_list); > > ctx->len = len; > > ctx->used = 0; > > - ctx->rcvused = 0; > > + atomic_set(&ctx->rcvused, 0); > > I think ATOMIC_INIT(0) is more suitable here. It's ugly to use it to assign a dynamic element like this. ctx->rcvused = (atomic_t)ATOMIC_INIT(0); Need the cast to avoid getting error: expected expression before '{' token #define ATOMIC_INIT(i) { (i) } There are only two drivers in the kernel doing this vs a lot doing initialization using the atomic_set option. I'm happy to change it though if you would prefer. > > > ctx->more = 0; > > ctx->merge = 0; > > ctx->enc = 0; > > diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c > > index c7c75ef41952..a5b4898f625a 100644 > > --- a/crypto/algif_skcipher.c > > +++ b/crypto/algif_skcipher.c > > @@ -388,7 +388,7 @@ static int skcipher_accept_parent_nokey(void *private, > > struct sock *sk) INIT_LIST_HEAD(&ctx->tsgl_list); > > ctx->len = len; > > ctx->used = 0; > > - ctx->rcvused = 0; > > + atomic_set(&ctx->rcvused, 0); > > dto. > > > ctx->more = 0; > > ctx->merge = 0; > > ctx->enc = 0; > > diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h > > index 38d9c5861ed8..f38227a78eae 100644 > > --- a/include/crypto/if_alg.h > > +++ b/include/crypto/if_alg.h > > @@ -18,6 +18,7 @@ > > #include <linux/if_alg.h> > > #include <linux/scatterlist.h> > > #include <linux/types.h> > > +#include <linux/atomic.h> > > #include <net/sock.h> > > > > #include <crypto/aead.h> > > @@ -150,7 +151,7 @@ struct af_alg_ctx { > > struct crypto_wait wait; > > > > size_t used; > > - size_t rcvused; > > + atomic_t rcvused; > > > > bool more; > > bool merge; > > @@ -215,7 +216,7 @@ static inline int af_alg_rcvbuf(struct sock *sk) > > struct af_alg_ctx *ctx = ask->private; > > > > return max_t(int, max_t(int, sk->sk_rcvbuf & PAGE_MASK, PAGE_SIZE) - > > - ctx->rcvused, 0); > > + atomic_read(&ctx->rcvused), 0); > > } > > > > /** > > Other than the comments above: > > Reviewed-by: Stephan Mueller <smueller@chronox.de> > > > Ciao > Stephan
Am Dienstag, 19. Dezember 2017, 12:26:42 CET schrieb Jonathan Cameron: Hi Jonathan, > > > + atomic_set(&ctx->rcvused, 0); > > > > I think ATOMIC_INIT(0) is more suitable here. > > It's ugly to use it to assign a dynamic element like this. > > ctx->rcvused = (atomic_t)ATOMIC_INIT(0); You are right, it is dynamic and not static assignment. Please disregard my comment. Ciao Stephan
On Tue, Dec 19, 2017 at 10:31:22AM +0000, Jonathan Cameron wrote: > This variable was increased and decreased without any protection. > Result was an occasional misscount and negative wrap around resulting > in false resource allocation failures. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code") Actually I think it used to be fine because we held the socket lock in the async callback. It only got broken when we removed the socket lock from the callback. commit 7d2c3f54e6f646887d019faa45f35d6fe9fe82ce Author: Stephan Mueller <smueller@chronox.de> Date: Fri Nov 10 13:20:55 2017 +0100 crypto: af_alg - remove locking in async callback Thanks,
Am Freitag, 22. Dezember 2017, 08:48:03 CET schrieb Herbert Xu: Hi Herbert, > On Tue, Dec 19, 2017 at 10:31:22AM +0000, Jonathan Cameron wrote: > > This variable was increased and decreased without any protection. > > Result was an occasional misscount and negative wrap around resulting > > in false resource allocation failures. > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code") > > Actually I think it used to be fine because we held the socket > lock in the async callback. It only got broken when we removed > the socket lock from the callback. But we cannot hold the lock in the async callback since it may be called in interrupt context. Ciao Stephan
On Fri, Dec 22, 2017 at 08:50:01AM +0100, Stephan Mueller wrote: > Am Freitag, 22. Dezember 2017, 08:48:03 CET schrieb Herbert Xu: > > Hi Herbert, > > > On Tue, Dec 19, 2017 at 10:31:22AM +0000, Jonathan Cameron wrote: > > > This variable was increased and decreased without any protection. > > > Result was an occasional misscount and negative wrap around resulting > > > in false resource allocation failures. > > > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code") > > > > Actually I think it used to be fine because we held the socket > > lock in the async callback. It only got broken when we removed > > the socket lock from the callback. > > But we cannot hold the lock in the async callback since it may be called in > interrupt context. I know. I'm just saying that the bug was introduced in the commit that removed the socket lock rather than the commit that introduced this originally. Cheers,
On Tue, Dec 19, 2017 at 10:31:22AM +0000, Jonathan Cameron wrote: > This variable was increased and decreased without any protection. > Result was an occasional misscount and negative wrap around resulting > in false resource allocation failures. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code") Patch applied. Thanks.
diff --git a/crypto/af_alg.c b/crypto/af_alg.c index 8612aa36a3ef..759cfa678c04 100644 --- a/crypto/af_alg.c +++ b/crypto/af_alg.c @@ -664,7 +664,7 @@ void af_alg_free_areq_sgls(struct af_alg_async_req *areq) unsigned int i; list_for_each_entry_safe(rsgl, tmp, &areq->rsgl_list, list) { - ctx->rcvused -= rsgl->sg_num_bytes; + atomic_sub(rsgl->sg_num_bytes, &ctx->rcvused); af_alg_free_sg(&rsgl->sgl); list_del(&rsgl->list); if (rsgl != &areq->first_rsgl) @@ -1173,7 +1173,7 @@ int af_alg_get_rsgl(struct sock *sk, struct msghdr *msg, int flags, areq->last_rsgl = rsgl; len += err; - ctx->rcvused += err; + atomic_add(err, &ctx->rcvused); rsgl->sg_num_bytes = err; iov_iter_advance(&msg->msg_iter, err); } diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c index db9378a45296..d3da3b0869f5 100644 --- a/crypto/algif_aead.c +++ b/crypto/algif_aead.c @@ -550,7 +550,7 @@ static int aead_accept_parent_nokey(void *private, struct sock *sk) INIT_LIST_HEAD(&ctx->tsgl_list); ctx->len = len; ctx->used = 0; - ctx->rcvused = 0; + atomic_set(&ctx->rcvused, 0); ctx->more = 0; ctx->merge = 0; ctx->enc = 0; diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c index c7c75ef41952..a5b4898f625a 100644 --- a/crypto/algif_skcipher.c +++ b/crypto/algif_skcipher.c @@ -388,7 +388,7 @@ static int skcipher_accept_parent_nokey(void *private, struct sock *sk) INIT_LIST_HEAD(&ctx->tsgl_list); ctx->len = len; ctx->used = 0; - ctx->rcvused = 0; + atomic_set(&ctx->rcvused, 0); ctx->more = 0; ctx->merge = 0; ctx->enc = 0; diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h index 38d9c5861ed8..f38227a78eae 100644 --- a/include/crypto/if_alg.h +++ b/include/crypto/if_alg.h @@ -18,6 +18,7 @@ #include <linux/if_alg.h> #include <linux/scatterlist.h> #include <linux/types.h> +#include <linux/atomic.h> #include <net/sock.h> #include <crypto/aead.h> @@ -150,7 +151,7 @@ struct af_alg_ctx { struct crypto_wait wait; size_t used; - size_t rcvused; + atomic_t rcvused; bool more; bool merge; @@ -215,7 +216,7 @@ static inline int af_alg_rcvbuf(struct sock *sk) struct af_alg_ctx *ctx = ask->private; return max_t(int, max_t(int, sk->sk_rcvbuf & PAGE_MASK, PAGE_SIZE) - - ctx->rcvused, 0); + atomic_read(&ctx->rcvused), 0); } /**
This variable was increased and decreased without any protection. Result was an occasional misscount and negative wrap around resulting in false resource allocation failures. Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code") --- The fixes tag is probably not ideal as I'm not 100% sure when this actually became a bug. The code in question was introduced in Fixes: e870456d8e7c ("crypto: algif_skcipher - overhaul memory management") and related. rcvused moved in Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code") So I have gone with the latter option as that is where it will cleanly apply. However, it probably doesn't matter as both are present only in the 4.14 final kernel. crypto/af_alg.c | 4 ++-- crypto/algif_aead.c | 2 +- crypto/algif_skcipher.c | 2 +- include/crypto/if_alg.h | 5 +++-- 4 files changed, 7 insertions(+), 6 deletions(-)