Date: Tue, 6 Nov 2018 16:17:37 -0800 (PST) From: "Rodney W. Grimes" <freebsd@pdx.rh.CN85.dnsmgr.net> To: Maxim Sobolev <sobomax@freebsd.org> Cc: rgrimes@freebsd.org, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r340187 - head/sys/geom Message-ID: <201811070017.wA70HbO2098370@pdx.rh.CN85.dnsmgr.net> In-Reply-To: <CAH7qZfvW2ZEqWL5yg9i67=zD562-KtY3tgcpSzCjku1JT3HGXQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
> Rodney, this was actually my original intention, however then I noticed in
> the GEOM code there is at least one case when BIO_FLUSH request is being
> generated internally with bio_offset == mediasize and bio_lenth == 0, so I
> thought there might be some need to allow such requests through. But I'd
> happily go with the stricter rule if it does no harm. I simply don't know
> enough about the intended use and the logic behind zero-length transfers to
> make that call.
I am not sure enough on if mediasize is 0 based or not,
if it is then the error case should be fixed, and the
code you show below should also be fixed as it is
technically making a request beyond the end of device.
I am also murky on why we are even doing a 0 size
operation and end of device, is that to validate
we can access all the media? If so then this wrong
code and wrong error return should be fixed as it
is off by 1.
>
> -Max
>
> int
> g_io_flush(struct g_consumer *cp)
> {
> ...
> bp = g_alloc_bio();
> bp->bio_cmd = BIO_FLUSH;
> ...
> bp->bio_offset = cp->provider->mediasize;
The above should have a - 1 on it.
> bp->bio_length = 0;
> bp->bio_data = NULL;
> g_io_request(bp, cp);
> ...
> }
>
>
>
> On Tue, Nov 6, 2018 at 8:31 AM Rodney W. Grimes <
> freebsd@pdx.rh.cn85.dnsmgr.net> wrote:
> >
> > > Author: sobomax
> > > Date: Tue Nov 6 15:55:41 2018
> > > New Revision: 340187
> > > URL: https://svnweb.freebsd.org/changeset/base/340187
> > >
> > > Log:
> > > Don't allow BIO_READ, BIO_WRITE or BIO_DELETE requests that are
> > > fully beyond the end of providers media. The only exception is made
> > > for the zero length transfers which are allowed to be just on the
> > > boundary. Previously, any requests starting on the boundary (i.e. next
> > > byte after the last one) have been allowed to go through.
> > >
> > > No response from: freebsd-geom@, phk
> > > MFC after: 1 month
> > >
> > > Modified:
> > > head/sys/geom/geom_io.c
> > >
> > > Modified: head/sys/geom/geom_io.c
> > >
> ==============================================================================
> > > --- head/sys/geom/geom_io.c Tue Nov 6 15:52:49 2018 (r340186)
> > > +++ head/sys/geom/geom_io.c Tue Nov 6 15:55:41 2018 (r340187)
> > > @@ -420,6 +420,8 @@ g_io_check(struct bio *bp)
> > > return (EIO);
> > > if (bp->bio_offset > pp->mediasize)
> > > return (EIO);
> > > + if (bp->bio_offset == pp->mediasize && bp->bio_length > 0)
> > > + return (EIO);
> >
> > Isnt mediasize 0 based, such that any operation at pp->mediasize is
> > technically past the end of the media and should get an EIO no
> > matter how big it is?
> >
> > Who is doing 0 byte operations at pp->mediasize?
> > That code should probably be fixed rather than allowing
> > this special case here.
> >
> > > /* Truncate requests to the end of providers media. */
> > > excess = bp->bio_offset + bp->bio_length;
> > >
> > >
> >
> > --
> > Rod Grimes
> rgrimes@freebsd.org
--
Rod Grimes rgrimes@freebsd.org
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201811070017.wA70HbO2098370>