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

[Andreas Gruenbacher <agruenba@redhat.com>]

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