[v2] iomap: Make sure iomap_end is called after iomap_begin
diff mbox series

Message ID 20200618122408.1054092-1-agruenba@redhat.com
State New
Headers show
Series
  • [v2] iomap: Make sure iomap_end is called after iomap_begin
Related show

Commit Message

Andreas Gruenbacher June 18, 2020, 12:24 p.m. UTC
Make sure iomap_end is always called when iomap_begin succeeds.

Without this fix, iomap_end won't be called when a filesystem's
iomap_begin operation returns an invalid mapping, bypassing any
unlocking done in iomap_end.  With this fix, the unlocking would
at least still happen.

This iomap_apply bug was found by Bob Peterson during code review.
It's unlikely that such iomap_begin bugs will survive to affect
users, so backporting this fix seems unnecessary.

Fixes: ae259a9c8593 ("fs: introduce iomap infrastructure")
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/iomap/apply.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)


base-commit: 69119673bd50b176ded34032fadd41530fb5af21

Comments

Dave Chinner June 18, 2020, 11:54 p.m. UTC | #1
On Thu, Jun 18, 2020 at 02:24:08PM +0200, Andreas Gruenbacher wrote:
> Make sure iomap_end is always called when iomap_begin succeeds.
> 
> Without this fix, iomap_end won't be called when a filesystem's
> iomap_begin operation returns an invalid mapping, bypassing any
> unlocking done in iomap_end.  With this fix, the unlocking would
> at least still happen.
> 
> This iomap_apply bug was found by Bob Peterson during code review.
> It's unlikely that such iomap_begin bugs will survive to affect
> users, so backporting this fix seems unnecessary.
> 
> Fixes: ae259a9c8593 ("fs: introduce iomap infrastructure")
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>

Thanks for the updated commit message, Andreas. :)

Patch looks good to me.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
Christoph Hellwig June 19, 2020, 1:13 p.m. UTC | #2
On Thu, Jun 18, 2020 at 02:24:08PM +0200, Andreas Gruenbacher wrote:
> Make sure iomap_end is always called when iomap_begin succeeds.
> 
> Without this fix, iomap_end won't be called when a filesystem's
> iomap_begin operation returns an invalid mapping, bypassing any
> unlocking done in iomap_end.  With this fix, the unlocking would
> at least still happen.
> 
> This iomap_apply bug was found by Bob Peterson during code review.
> It's unlikely that such iomap_begin bugs will survive to affect
> users, so backporting this fix seems unnecessary.
> 
> Fixes: ae259a9c8593 ("fs: introduce iomap infrastructure")
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> ---
>  fs/iomap/apply.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/iomap/apply.c b/fs/iomap/apply.c
> index 76925b40b5fd..32daf8cb411c 100644
> --- a/fs/iomap/apply.c
> +++ b/fs/iomap/apply.c
> @@ -46,10 +46,11 @@ iomap_apply(struct inode *inode, loff_t pos, loff_t length, unsigned flags,
>  	ret = ops->iomap_begin(inode, pos, length, flags, &iomap, &srcmap);
>  	if (ret)
>  		return ret;
> -	if (WARN_ON(iomap.offset > pos))
> -		return -EIO;
> -	if (WARN_ON(iomap.length == 0))
> -		return -EIO;
> +	if (WARN_ON(iomap.offset > pos) ||
> +	    WARN_ON(iomap.length == 0)) {
> +		written = -EIO;
> +		goto out;
> +	}

As said before please don't merge these for no good reason.
Andreas Gruenbacher June 22, 2020, 9:07 a.m. UTC | #3
On Fri, Jun 19, 2020 at 3:25 PM Christoph Hellwig <hch@infradead.org> wrote:
> On Thu, Jun 18, 2020 at 02:24:08PM +0200, Andreas Gruenbacher wrote:
> > Make sure iomap_end is always called when iomap_begin succeeds.
> >
> > Without this fix, iomap_end won't be called when a filesystem's
> > iomap_begin operation returns an invalid mapping, bypassing any
> > unlocking done in iomap_end.  With this fix, the unlocking would
> > at least still happen.
> >
> > This iomap_apply bug was found by Bob Peterson during code review.
> > It's unlikely that such iomap_begin bugs will survive to affect
> > users, so backporting this fix seems unnecessary.
> >
> > Fixes: ae259a9c8593 ("fs: introduce iomap infrastructure")
> > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> > ---
> >  fs/iomap/apply.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/iomap/apply.c b/fs/iomap/apply.c
> > index 76925b40b5fd..32daf8cb411c 100644
> > --- a/fs/iomap/apply.c
> > +++ b/fs/iomap/apply.c
> > @@ -46,10 +46,11 @@ iomap_apply(struct inode *inode, loff_t pos, loff_t length, unsigned flags,
> >       ret = ops->iomap_begin(inode, pos, length, flags, &iomap, &srcmap);
> >       if (ret)
> >               return ret;
> > -     if (WARN_ON(iomap.offset > pos))
> > -             return -EIO;
> > -     if (WARN_ON(iomap.length == 0))
> > -             return -EIO;
> > +     if (WARN_ON(iomap.offset > pos) ||
> > +         WARN_ON(iomap.length == 0)) {
> > +             written = -EIO;
> > +             goto out;
> > +     }
>
> As said before please don't merge these for no good reason.

I really didn't expect this tiny patch to require much discussion at
all, but just to be clear ... do you actually object to this very
patch that explicitly doesn't merge the two checks and keeps them on
two separate lines so that the warning messages will report different
line numbers, or are you fine with that?

Thanks,
Andreas
Christoph Hellwig June 23, 2020, 10:36 a.m. UTC | #4
On Mon, Jun 22, 2020 at 11:07:59AM +0200, Andreas Gruenbacher wrote:
> On Fri, Jun 19, 2020 at 3:25 PM Christoph Hellwig <hch@infradead.org> wrote:
> > On Thu, Jun 18, 2020 at 02:24:08PM +0200, Andreas Gruenbacher wrote:
> > > Make sure iomap_end is always called when iomap_begin succeeds.
> > >
> > > Without this fix, iomap_end won't be called when a filesystem's
> > > iomap_begin operation returns an invalid mapping, bypassing any
> > > unlocking done in iomap_end.  With this fix, the unlocking would
> > > at least still happen.
> > >
> > > This iomap_apply bug was found by Bob Peterson during code review.
> > > It's unlikely that such iomap_begin bugs will survive to affect
> > > users, so backporting this fix seems unnecessary.
> > >
> > > Fixes: ae259a9c8593 ("fs: introduce iomap infrastructure")
> > > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> > > ---
> > >  fs/iomap/apply.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/iomap/apply.c b/fs/iomap/apply.c
> > > index 76925b40b5fd..32daf8cb411c 100644
> > > --- a/fs/iomap/apply.c
> > > +++ b/fs/iomap/apply.c
> > > @@ -46,10 +46,11 @@ iomap_apply(struct inode *inode, loff_t pos, loff_t length, unsigned flags,
> > >       ret = ops->iomap_begin(inode, pos, length, flags, &iomap, &srcmap);
> > >       if (ret)
> > >               return ret;
> > > -     if (WARN_ON(iomap.offset > pos))
> > > -             return -EIO;
> > > -     if (WARN_ON(iomap.length == 0))
> > > -             return -EIO;
> > > +     if (WARN_ON(iomap.offset > pos) ||
> > > +         WARN_ON(iomap.length == 0)) {
> > > +             written = -EIO;
> > > +             goto out;
> > > +     }
> >
> > As said before please don't merge these for no good reason.
> 
> I really didn't expect this tiny patch to require much discussion at
> all, but just to be clear ... do you actually object to this very
> patch that explicitly doesn't merge the two checks and keeps them on
> two separate lines so that the warning messages will report different
> line numbers, or are you fine with that?

Yes, it merges the WARN_ONs, and thus reduces their usefulness.  How
about a patch that just fixes your reported issue insted of messing up
other things for no good reason?
Andreas Gr├╝nbacher June 23, 2020, 10:51 a.m. UTC | #5
Am Di., 23. Juni 2020 um 12:38 Uhr schrieb Christoph Hellwig
<hch@infradead.org>:
> On Mon, Jun 22, 2020 at 11:07:59AM +0200, Andreas Gruenbacher wrote:
> > On Fri, Jun 19, 2020 at 3:25 PM Christoph Hellwig <hch@infradead.org> wrote:
> > > On Thu, Jun 18, 2020 at 02:24:08PM +0200, Andreas Gruenbacher wrote:
> > > > Make sure iomap_end is always called when iomap_begin succeeds.
> > > >
> > > > Without this fix, iomap_end won't be called when a filesystem's
> > > > iomap_begin operation returns an invalid mapping, bypassing any
> > > > unlocking done in iomap_end.  With this fix, the unlocking would
> > > > at least still happen.
> > > >
> > > > This iomap_apply bug was found by Bob Peterson during code review.
> > > > It's unlikely that such iomap_begin bugs will survive to affect
> > > > users, so backporting this fix seems unnecessary.
> > > >
> > > > Fixes: ae259a9c8593 ("fs: introduce iomap infrastructure")
> > > > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> > > > ---
> > > >  fs/iomap/apply.c | 10 ++++++----
> > > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/fs/iomap/apply.c b/fs/iomap/apply.c
> > > > index 76925b40b5fd..32daf8cb411c 100644
> > > > --- a/fs/iomap/apply.c
> > > > +++ b/fs/iomap/apply.c
> > > > @@ -46,10 +46,11 @@ iomap_apply(struct inode *inode, loff_t pos, loff_t length, unsigned flags,
> > > >       ret = ops->iomap_begin(inode, pos, length, flags, &iomap, &srcmap);
> > > >       if (ret)
> > > >               return ret;
> > > > -     if (WARN_ON(iomap.offset > pos))
> > > > -             return -EIO;
> > > > -     if (WARN_ON(iomap.length == 0))
> > > > -             return -EIO;
> > > > +     if (WARN_ON(iomap.offset > pos) ||
> > > > +         WARN_ON(iomap.length == 0)) {
> > > > +             written = -EIO;
> > > > +             goto out;
> > > > +     }
> > >
> > > As said before please don't merge these for no good reason.
> >
> > I really didn't expect this tiny patch to require much discussion at
> > all, but just to be clear ... do you actually object to this very
> > patch that explicitly doesn't merge the two checks and keeps them on
> > two separate lines so that the warning messages will report different
> > line numbers, or are you fine with that?
>
> Yes, it merges the WARN_ONs, and thus reduces their usefulness.  How
> about a patch that just fixes your reported issue insted of messing up
> other things for no good reason?

So you're saying you prefer this:

+       if (WARN_ON(iomap.offset > pos)) {
+               written = -EIO;
+               goto out;
+       }
+       if (WARN_ON(iomap.length == 0)) {
+               written = -EIO;
+               goto out;
+       }

to this:

+       if (WARN_ON(iomap.offset > pos) ||
+           WARN_ON(iomap.length == 0)) {
+               written = -EIO;
+               goto out;
+       }

Well fine, you don't need to accuse me of messing up things for that.

Andreas
Christoph Hellwig June 23, 2020, 11:32 a.m. UTC | #6
On Tue, Jun 23, 2020 at 12:51:00PM +0200, Andreas Gr??nbacher wrote:
> > Yes, it merges the WARN_ONs, and thus reduces their usefulness.  How
> > about a patch that just fixes your reported issue insted of messing up
> > other things for no good reason?
> 
> So you're saying you prefer this:
> 
> +       if (WARN_ON(iomap.offset > pos)) {
> +               written = -EIO;
> +               goto out;
> +       }
> +       if (WARN_ON(iomap.length == 0)) {
> +               written = -EIO;
> +               goto out;
> +       }
> 
> to this:
> 
> +       if (WARN_ON(iomap.offset > pos) ||
> +           WARN_ON(iomap.length == 0)) {
> +               written = -EIO;
> +               goto out;
> +       }
> 
> Well fine, you don't need to accuse me of messing up things for that.

Yes.  And we had discussion on exactly that on the previous iteration..

Patch
diff mbox series

diff --git a/fs/iomap/apply.c b/fs/iomap/apply.c
index 76925b40b5fd..32daf8cb411c 100644
--- a/fs/iomap/apply.c
+++ b/fs/iomap/apply.c
@@ -46,10 +46,11 @@  iomap_apply(struct inode *inode, loff_t pos, loff_t length, unsigned flags,
 	ret = ops->iomap_begin(inode, pos, length, flags, &iomap, &srcmap);
 	if (ret)
 		return ret;
-	if (WARN_ON(iomap.offset > pos))
-		return -EIO;
-	if (WARN_ON(iomap.length == 0))
-		return -EIO;
+	if (WARN_ON(iomap.offset > pos) ||
+	    WARN_ON(iomap.length == 0)) {
+		written = -EIO;
+		goto out;
+	}
 
 	trace_iomap_apply_dstmap(inode, &iomap);
 	if (srcmap.type != IOMAP_HOLE)
@@ -80,6 +81,7 @@  iomap_apply(struct inode *inode, loff_t pos, loff_t length, unsigned flags,
 	written = actor(inode, pos, length, data, &iomap,
 			srcmap.type != IOMAP_HOLE ? &srcmap : &iomap);
 
+out:
 	/*
 	 * Now the data has been copied, commit the range we've copied.  This
 	 * should not fail unless the filesystem has had a fatal error.