[PATCH v2] iomap: Make sure iomap_end is called after iomap_begin

[PATCH v2] iomap: Make sure iomap_end is called after iomap_begin

[Andreas Gruenbacher]

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;
+	}
 
 	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.

base-commit: 69119673bd50b176ded34032fadd41530fb5af21
-- 
2.26.2

Re: [PATCH v2] iomap: Make sure iomap_end is called after iomap_begin

[Dave Chinner]

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>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] iomap: Make sure iomap_end is called after iomap_begin

[Christoph Hellwig]

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.

Re: [PATCH v2] iomap: Make sure iomap_end is called after iomap_begin

[Andreas Gruenbacher]

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

Re: [PATCH v2] iomap: Make sure iomap_end is called after iomap_begin

[Christoph Hellwig]

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?

Re: [PATCH v2] iomap: Make sure iomap_end is called after iomap_begin

[Andreas Grünbacher]

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

Re: [PATCH v2] iomap: Make sure iomap_end is called after iomap_begin

[Christoph Hellwig]

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..