From: Ming Lei <tom.leiming@gmail.com>
To: Dave Chinner <david@fromorbit.com>
Cc: "open list:XFS FILESYSTEM" <linux-xfs@vger.kernel.org>,
linux-block <linux-block@vger.kernel.org>
Subject: Re: [PATCH] block: fix 32 bit overflow in __blkdev_issue_discard()
Date: Wed, 14 Nov 2018 16:15:26 +0800 [thread overview]
Message-ID: <CACVXFVNCvinW7eDaOtpGTrpiv+6z4Cjn=tsjuHoY8emphF6-1g@mail.gmail.com> (raw)
In-Reply-To: <20181114080859.GU19305@dastard>
On Wed, Nov 14, 2018 at 4:09 PM Dave Chinner <david@fromorbit.com> wrote:
>
> On Wed, Nov 14, 2018 at 10:53:11AM +0800, Ming Lei wrote:
> > On Wed, Nov 14, 2018 at 5:44 AM Dave Chinner <david@fromorbit.com> wrote:
> > >
> > > From: Dave Chinner <dchinner@redhat.com>
> > >
> > > A discard cleanup merged into 4.20-rc2 causes fstests xfs/259 to
> > > fall into an endless loop in the discard code. The test is creating
> > > a device that is exactly 2^32 sectors in size to test mkfs boundary
> > > conditions around the 32 bit sector overflow region.
> > >
> > > mkfs issues a discard for the entire device size by default, and
> > > hence this throws a sector count of 2^32 into
> > > blkdev_issue_discard(). It takes the number of sectors to discard as
> > > a sector_t - a 64 bit value.
> > >
> > > The commit ba5d73851e71 ("block: cleanup __blkdev_issue_discard")
> > > takes this sector count and casts it to a 32 bit value before
> > > comapring it against the maximum allowed discard size the device
> > > has. This truncates away the upper 32 bits, and so if the lower 32
> > > bits of the sector count is zero, it starts issuing discards of
> > > length 0. This causes the code to fall into an endless loop, issuing
> > > a zero length discards over and over again on the same sector.
> > >
> > > Fixes: ba5d73851e71 ("block: cleanup __blkdev_issue_discard")
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > ---
> > > block/blk-lib.c | 5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/block/blk-lib.c b/block/blk-lib.c
> > > index e8b3bb9bf375..144e156ed341 100644
> > > --- a/block/blk-lib.c
> > > +++ b/block/blk-lib.c
> > > @@ -55,9 +55,12 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
> > > return -EINVAL;
> > >
> > > while (nr_sects) {
> > > - unsigned int req_sects = min_t(unsigned int, nr_sects,
> > > + sector_t req_sects = min_t(sector_t, nr_sects,
> > > bio_allowed_max_sectors(q));
> >
> > bio_allowed_max_sectors(q) is always < UINT_MAX, and 'sector_t' is only
> > required during the comparison, so another simpler fix might be the following,
> > could you test if it is workable?
> >
> > diff --git a/block/blk-lib.c b/block/blk-lib.c
> > index e8b3bb9bf375..6ef44f99e83f 100644
> > --- a/block/blk-lib.c
> > +++ b/block/blk-lib.c
> > @@ -55,7 +55,7 @@ int __blkdev_issue_discard(struct block_device
> > *bdev, sector_t sector,
> > return -EINVAL;
> >
> > while (nr_sects) {
> > - unsigned int req_sects = min_t(unsigned int, nr_sects,
> > + unsigned int req_sects = min_t(sector_t, nr_sects,
> > bio_allowed_max_sectors(q));
>
> Rearrange the deck chairs all you like, just make sure you fix your
> regression test suite to exercise obvious boundary conditions like
> this so the next cleanup doesn't break the code again.
Good point, we may add comment on the overflow story.
>
> > >
> > > + WARN_ON_ONCE(req_sects == 0);
> >
> > The above line isn't necessary given 'nr_sects' can't be zero.
>
> Except it was 0 and it caused the bug I had to fix. So it should
> have a warning on it.
Obviously, it can't be zero except CPU is broken, :-)
Thanks,
Ming Lei
next prev parent reply other threads:[~2018-11-14 18:17 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-13 21:43 [PATCH] block: fix 32 bit overflow in __blkdev_issue_discard() Dave Chinner
2018-11-14 2:36 ` Darrick J. Wong
2018-11-14 2:53 ` Ming Lei
2018-11-14 8:08 ` Dave Chinner
2018-11-14 8:15 ` Ming Lei [this message]
2018-11-14 15:18 ` Jens Axboe
2018-11-15 1:06 ` Ming Lei
2018-11-15 1:22 ` Dave Chinner
2018-11-15 3:10 ` Ming Lei
2018-11-15 22:13 ` Dave Chinner
2018-11-15 22:24 ` Darrick J. Wong
2018-11-16 4:04 ` Dave Chinner
2018-11-16 8:32 ` Christoph Hellwig
2018-11-16 8:46 ` Omar Sandoval
2018-11-16 8:53 ` Christoph Hellwig
2018-11-16 12:06 ` Ming Lei
2018-11-15 1:51 ` Jens Axboe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CACVXFVNCvinW7eDaOtpGTrpiv+6z4Cjn=tsjuHoY8emphF6-1g@mail.gmail.com' \
--to=tom.leiming@gmail.com \
--cc=david@fromorbit.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).