[bug report] xen-blkback: don't "handle" error by BUG()

[bug report] xen-blkback: don't "handle" error by BUG()

[Dan Carpenter]

Hello Jan Beulich,

The patch 5a264285ed1c: "xen-blkback: don't "handle" error by BUG()"
from Feb 15, 2021, leads to the following static checker warning:

	drivers/block/xen-blkback/blkback.c:836 xen_blkbk_map()
	warn: should this be a bitwise negate mask?

drivers/block/xen-blkback/blkback.c
   823           * Now swizzle the MFN in our domain with the MFN from the other domain
   824           * so that when we access vaddr(pending_req,i) it has the contents of
   825           * the page from the other domain.
   826           */
   827          for (seg_idx = last_map, new_map_idx = 0; seg_idx < map_until; seg_idx++) {
   828                  if (!pages[seg_idx]->persistent_gnt) {
   829                          /* This is a newly mapped grant */
   830                          BUG_ON(new_map_idx >= segs_to_map);
   831                          if (unlikely(map[new_map_idx].status != 0)) {
   832                                  pr_debug("invalid buffer -- could not remap it\n");
   833                                  gnttab_page_cache_put(&ring->free_pages,
   834                                                        &pages[seg_idx]->page, 1);
   835                                  pages[seg_idx]->handle = BLKBACK_INVALID_HANDLE;
   836                                  ret |= !ret;

Originally this code was:

	ret |= 1;

Now it's equivalent to:

	if (!ret)
		ret = 1;

This is the second user of this idiom in the kernel.  Both were
introduced in the last month so maybe it's a new trend or something...
Anyway.  False positive warning.

But my main question isn't really related to this patch.  What does
the 1 mean in this context?  I always feel like there should be
documentation when functions return a mix of error codes, zero and one
but there isn't any here.

I have reviewed this and so far as I can see setting "ret = 1;" is
always treated exactly the same as an error code by everything.  Every
single place where this is checked just checks for ret is zero.  The
value is propagated two functions back and then it becomes -EIO.

???

   837                                  goto next;
   838                          }
   839                          pages[seg_idx]->handle = map[new_map_idx].handle;
   840                  } else {
   841                          continue;
   842                  }
   843                  if (use_persistent_gnts &&

regards,
dan carpenter

Re: [bug report] xen-blkback: don't "handle" error by BUG()

[Jan Beulich]

On 19.02.2021 09:59, Dan Carpenter wrote:
> Hello Jan Beulich,
> 
> The patch 5a264285ed1c: "xen-blkback: don't "handle" error by BUG()"
> from Feb 15, 2021, leads to the following static checker warning:
> 
> 	drivers/block/xen-blkback/blkback.c:836 xen_blkbk_map()
> 	warn: should this be a bitwise negate mask?
> 
> drivers/block/xen-blkback/blkback.c
>    823           * Now swizzle the MFN in our domain with the MFN from the other domain
>    824           * so that when we access vaddr(pending_req,i) it has the contents of
>    825           * the page from the other domain.
>    826           */
>    827          for (seg_idx = last_map, new_map_idx = 0; seg_idx < map_until; seg_idx++) {
>    828                  if (!pages[seg_idx]->persistent_gnt) {
>    829                          /* This is a newly mapped grant */
>    830                          BUG_ON(new_map_idx >= segs_to_map);
>    831                          if (unlikely(map[new_map_idx].status != 0)) {
>    832                                  pr_debug("invalid buffer -- could not remap it\n");
>    833                                  gnttab_page_cache_put(&ring->free_pages,
>    834                                                        &pages[seg_idx]->page, 1);
>    835                                  pages[seg_idx]->handle = BLKBACK_INVALID_HANDLE;
>    836                                  ret |= !ret;
> 
> Originally this code was:
> 
> 	ret |= 1;
> 
> Now it's equivalent to:
> 
> 	if (!ret)
> 		ret = 1;
> 
> This is the second user of this idiom in the kernel.  Both were
> introduced in the last month so maybe it's a new trend or something...
> Anyway.  False positive warning.
> 
> But my main question isn't really related to this patch.  What does
> the 1 mean in this context?  I always feel like there should be
> documentation when functions return a mix of error codes, zero and one
> but there isn't any here.

I agree. The literal 1 was there before, and the security fix
was not a good place to change this. I suppose you really
should ask whoever introduced that use of literal 1. I find
this as odd as you do, and ...

> I have reviewed this and so far as I can see setting "ret = 1;" is
> always treated exactly the same as an error code by everything.  Every
> single place where this is checked just checks for ret is zero.  The
> value is propagated two functions back and then it becomes -EIO.

... I've come to the same conclusions.

Jan