| Message ID | 20200618122408.1054092-1-agruenba@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] iomap: Make sure iomap_end is called after iomap_begin | expand |
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>
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.
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
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?
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
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..
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.
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