Re: block: fail unaligned bio from submit_bio_noacct()

[Ming Lei <ming.lei@redhat.com>]

On Thu, Mar 21, 2024 at 06:01:41PM +0100, Mikulas Patocka wrote:
> 
> 
> On Thu, 21 Mar 2024, Mike Snitzer wrote:
> 
> > On Thu, Mar 21 2024 at  9:16P -0400,
> > Ming Lei <ming.lei@redhat.com> wrote:
> > 
> > > For any bio with data, its start sector and size have to be aligned with
> > > the queue's logical block size.
> > > 
> > > This rule is obvious, but there is still user which may send unaligned
> > > bio to block layer, and it is observed that dm-integrity can do that,
> > > and cause double free of driver's dma meta buffer.
> > > 
> > > So failfast unaligned bio from submit_bio_noacct() for avoiding more
> > > troubles.
> > > 
> > > Cc: Mikulas Patocka <mpatocka@redhat.com>
> > > Cc: Mike Snitzer <snitzer@kernel.org>
> > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > ---
> > >  block/blk-core.c | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/block/blk-core.c b/block/blk-core.c
> > > index a16b5abdbbf5..b1a10187ef74 100644
> > > --- a/block/blk-core.c
> > > +++ b/block/blk-core.c
> > > @@ -729,6 +729,20 @@ void submit_bio_noacct_nocheck(struct bio *bio)
> > >  		__submit_bio_noacct(bio);
> > >  }
> > >  
> > > +static bool bio_check_alignment(struct bio *bio, struct request_queue *q)
> > > +{
> > > +	unsigned int bs = q->limits.logical_block_size;
> > > +	unsigned int size = bio->bi_iter.bi_size;
> > > +
> > > +	if (size & (bs - 1))
> > > +		return false;
> > > +
> > > +	if (size && ((bio->bi_iter.bi_sector << SECTOR_SHIFT) & (bs - 1)))
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> 
> I would change it to
> 
> if (unlikely(((bi_iter.bi_sector | bio_sectors(bio)) & ((queue_logical_block_size(q) >> 9) - 1)) != 0))
> 	return false;

What if bio->bi_iter.bi_size isn't aligned with 512? The above check
can't find that at all.

> 
> > >  /**
> > >   * submit_bio_noacct - re-submit a bio to the block device layer for I/O
> > >   * @bio:  The bio describing the location in memory and on the device.
> > > @@ -780,6 +794,9 @@ void submit_bio_noacct(struct bio *bio)
> > >  		}
> > >  	}
> > >  
> > > +	if (WARN_ON_ONCE(!bio_check_alignment(bio, q)))
> > > +		goto end_io;
> > > +
> > >  	if (!test_bit(QUEUE_FLAG_POLL, &q->queue_flags))
> > >  		bio_clear_polled(bio);
> > >  
> > > -- 
> > > 2.41.0
> > > 
> > > 
> > 
> > This check would really help more quickly find buggy code, but it
> > would be unfortunate for these extra checks to be required in
> > production.  It feels like this is the type of check that should be
> > wrapped by a debug CONFIG option (so only debug kernels have it).
> > 
> > Do we already have an appropriate CONFIG option to use?
> > 
> > Mike
> 
> But then, the system would crash with the config option being 'n' and 
> return an error with the config option being 'y' - which would be 
> unfortunate.

Yes, the check is basically zero-cost, not necessary to add config to
make things more complicated.

Thanks,
Ming