[PATCH] xarray: unlock on error in xa_alloc()

[PATCH] xarray: unlock on error in xa_alloc()

[Dan Carpenter]

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.

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);

Re: [PATCH] xarray: unlock on error in xa_alloc()

[Matthew Wilcox]

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);

Re: [PATCH] xarray: unlock on error in xa_alloc()

[Dan Carpenter]

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

Re: [PATCH] xarray: unlock on error in xa_alloc()

[Matthew Wilcox]

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.

Re: [PATCH] xarray: unlock on error in xa_alloc()

[Dan Carpenter]

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