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

[Jan Beulich <jbeulich@xxxxxxxx>]

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