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@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;
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
