Re: [PATCH] block: avoid sign extend problem with default queue flags mask

[Brian Foster <bfoster@redhat.com>]

On Fri, Sep 30, 2022 at 03:33:29PM -0600, Jens Axboe wrote:
> On 9/30/22 9:03 AM, Brian Foster wrote:
> > request_queue->queue_flags is an 8-byte field. Most queue flag
> > modifications occur through bit field helpers, but default flags can
> > be logically OR'd via the QUEUE_FLAG_MQ_DEFAULT mask. If this mask
> > happens to include bit 31, the assignment can sign extend the field
> > and set all upper 32 bits.
> > 
> > This exact problem has been observed on a downstream kernel that
> > happens to use bit 31 for QUEUE_FLAG_NOWAIT. This is not an
> > immediate problem for current upstream because bit 31 is not
> > included in the default flag assignment (and is not used at all,
> > actually). Regardless, fix up the QUEUE_FLAG_MQ_DEFAULT mask
> > definition to avoid the landmine in the future.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> > 
> > Just to elaborate, I ran a quick test to change QUEUE_FLAG_NOWAIT to use
> > bit 31. With that change but without this patch, I see the following
> > queue state:
> > 
> > # cat /sys/kernel/debug/block/vda/state
> > SAME_COMP|IO_STAT|INIT_DONE|WC|STATS|REGISTERED|30|NOWAIT|32|33|34|35|36|37|38|39|40|41|42|43|44|45|46|47|48|49|50|51|52|53|54|55|56|57|58|59|60|61|62|63
> > 
> > And then with the patch applied:
> > 
> > # cat /sys/kernel/debug/block/vda/state
> > SAME_COMP|IO_STAT|INIT_DONE|WC|STATS|REGISTERED|30|NOWAIT
> > 
> > Thanks.
> > 
> > Brian
> > 
> >  include/linux/blkdev.h | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > index 84b13fdd34a7..28c3037cb25c 100644
> > --- a/include/linux/blkdev.h
> > +++ b/include/linux/blkdev.h
> > @@ -580,9 +580,9 @@ struct request_queue {
> >  #define QUEUE_FLAG_NOWAIT       29	/* device supports NOWAIT */
> >  #define QUEUE_FLAG_SQ_SCHED     30	/* single queue style io dispatch */
> >  
> > -#define QUEUE_FLAG_MQ_DEFAULT	((1 << QUEUE_FLAG_IO_STAT) |		\
> > -				 (1 << QUEUE_FLAG_SAME_COMP) |		\
> > -				 (1 << QUEUE_FLAG_NOWAIT))
> > +#define QUEUE_FLAG_MQ_DEFAULT	((1ULL << QUEUE_FLAG_IO_STAT) |		\
> > +				 (1ULL << QUEUE_FLAG_SAME_COMP) |	\
> > +				 (1ULL << QUEUE_FLAG_NOWAIT))
> 
> Shouldn't this just be 1UL << foo? The queue_flags are not 8-bytes,
> they are unsigned long. That happens to be 8-bytes on 64-bit archs,
> but it's 4-bytes on 32-bit archs.
> 

Oops.. yes, that makes sense. I guess that means we shouldn't really
expect to see anything use the upper 32 bits. The extension still makes
the state output look wonky in the (1 << 31) case, so I'll send a v2
with that fixed..

Brian

> -- 
> Jens Axboe
> 
>