On Thu, 21 Mar 2024, Mike Snitzer wrote:
> On Thu, Mar 21 2024 at 9:16P -0400,
> Ming Lei <ming.lei@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Cc: Mike Snitzer <snitzer@xxxxxxxxxx>
> > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> > ---
> > 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;
> > /**
> > * 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.
We could remove the check from the drivers and add it to the generic I/O
path - this wouldn't add extra overhead.
Mikulas
