| Message ID | 20180706172101.vvp3fv3l244y2p7w@kili.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 06, 2018 at 08:21:01PM +0300, Dan Carpenter wrote: > We need to unlock on this error path. > > Fixes: 29a6bfc32eb2 ("xarray: Track free entries in an XArray") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > > There "UINT_MAX + 1" is an integer overflow and is equal to zero but I > don't know what was intended there. Ah. I didn't realise UINT_MAX was defined as ~0U. I had intended UINT_MAX + 1UL. ie 0x10000000UL on 64-bit and 0 on 32-bit. > diff --git a/lib/xarray.c b/lib/xarray.c > index be10039caaed..a27fdb381f64 100644 > --- a/lib/xarray.c > +++ b/lib/xarray.c > @@ -1474,8 +1474,10 @@ int xa_alloc(struct xarray *xa, u32 *id, void *entry, gfp_t gfp) > xas.xa_index = 0; > xas_lock(&xas); > xas_find_tagged(&xas, UINT_MAX, XA_FREE_TAG); > - if (xas.xa_node == XAS_BOUNDS && xas.xa_index == UINT_MAX + 1) > + if (xas.xa_node == XAS_BOUNDS && xas.xa_index == UINT_MAX + 1) { > + xas_unlock(&xas); > return -ENOSPC; > + } > *id = xas.xa_index; > xas_store(&xas, entry); > xas_clear_tag(&xas, XA_FREE_TAG);
On Fri, Jul 06, 2018 at 10:51:30AM -0700, Matthew Wilcox wrote: > On Fri, Jul 06, 2018 at 08:21:01PM +0300, Dan Carpenter wrote: > > We need to unlock on this error path. > > > > Fixes: 29a6bfc32eb2 ("xarray: Track free entries in an XArray") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > > > There "UINT_MAX + 1" is an integer overflow and is equal to zero but I > > don't know what was intended there. > > Ah. I didn't realise UINT_MAX was defined as ~0U. I had intended > UINT_MAX + 1UL. ie 0x10000000UL on 64-bit and 0 on 32-bit. > I will push a Smatch check so that the wrap around on 32 bit systems will generate a warning. regards, dan carpenter
On Fri, Jul 06, 2018 at 09:26:46PM +0300, Dan Carpenter wrote: > On Fri, Jul 06, 2018 at 10:51:30AM -0700, Matthew Wilcox wrote: > > On Fri, Jul 06, 2018 at 08:21:01PM +0300, Dan Carpenter wrote: > > > We need to unlock on this error path. > > > > > > Fixes: 29a6bfc32eb2 ("xarray: Track free entries in an XArray") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > --- > > > > > > There "UINT_MAX + 1" is an integer overflow and is equal to zero but I > > > don't know what was intended there. > > > > Ah. I didn't realise UINT_MAX was defined as ~0U. I had intended > > UINT_MAX + 1UL. ie 0x10000000UL on 64-bit and 0 on 32-bit. > > > > I will push a Smatch check so that the wrap around on 32 bit systems > will generate a warning. Do you mean "on 64-bit systems"? Because the code as-written was correct on 32-bit systems and buggy on 64-bit systems. I suppose generally, this is: if (unsigned long == unsigned int + int) where we can tell statically that the right hand side will wrap on 32-bit systems and not on 64-bit systems.
On Fri, Jul 06, 2018 at 12:08:14PM -0700, Matthew Wilcox wrote: > On Fri, Jul 06, 2018 at 09:26:46PM +0300, Dan Carpenter wrote: > > On Fri, Jul 06, 2018 at 10:51:30AM -0700, Matthew Wilcox wrote: > > > On Fri, Jul 06, 2018 at 08:21:01PM +0300, Dan Carpenter wrote: > > > > We need to unlock on this error path. > > > > > > > > Fixes: 29a6bfc32eb2 ("xarray: Track free entries in an XArray") > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > --- > > > > > > > > There "UINT_MAX + 1" is an integer overflow and is equal to zero but I > > > > don't know what was intended there. > > > > > > Ah. I didn't realise UINT_MAX was defined as ~0U. I had intended > > > UINT_MAX + 1UL. ie 0x10000000UL on 64-bit and 0 on 32-bit. > > > > > > > I will push a Smatch check so that the wrap around on 32 bit systems > > will generate a warning. > > Do you mean "on 64-bit systems"? Because the code as-written was correct > on 32-bit systems and buggy on 64-bit systems. > > I suppose generally, this is: > > if (unsigned long == unsigned int + int) > > where we can tell statically that the right hand side will wrap on 32-bit > systems and not on 64-bit systems. No no. I'm going to print a warning any time you add two numbers together and it wraps around. It's normally a bug. One idea to silence the warning would be to use #ifdef 64BIT #else. regards, dan carpenter
diff --git a/lib/xarray.c b/lib/xarray.c index be10039caaed..a27fdb381f64 100644 --- a/lib/xarray.c +++ b/lib/xarray.c @@ -1474,8 +1474,10 @@ int xa_alloc(struct xarray *xa, u32 *id, void *entry, gfp_t gfp) xas.xa_index = 0; xas_lock(&xas); xas_find_tagged(&xas, UINT_MAX, XA_FREE_TAG); - if (xas.xa_node == XAS_BOUNDS && xas.xa_index == UINT_MAX + 1) + if (xas.xa_node == XAS_BOUNDS && xas.xa_index == UINT_MAX + 1) { + xas_unlock(&xas); return -ENOSPC; + } *id = xas.xa_index; xas_store(&xas, entry); xas_clear_tag(&xas, XA_FREE_TAG);
We need to unlock on this error path. Fixes: 29a6bfc32eb2 ("xarray: Track free entries in an XArray") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- There "UINT_MAX + 1" is an integer overflow and is equal to zero but I don't know what was intended there.