| Message ID | YzXnoR0UMBVfoaOf@magnolia (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | iomap: fix memory corruption when recording errors during writeback | expand |
On Thu, Sep 29, 2022 at 11:44:49AM -0700, Darrick J. Wong wrote: > Fixes: e735c0079465 ("iomap: Convert iomap_add_to_ioend() to take a folio") > Probably-Fixes: 598ecfbaa742 ("iomap: lift the xfs writeback code to iomap") I think this is a misuse of Fixes. As I understand it, Fixes: is "Here's the commit that introduced the bug", not "This is the most recent change after which this patch will still apply". e735c0079465 only changed s/page/folio/ in this line of code, so clearly didn't introduce the bug. Any kernel containing 598ecfbaa742 has this same bug, so that should be the Fixes: line. As you say though, 598ecfbaa742 merely moved the code from xfs_writepage_map(). bfce7d2e2d5e moved it from xfs_do_writepage(), but 150d5be09ce4 introduced it. Six years ago! Good find. So how about: Fixes: 150d5be09ce4 ("xfs: remove xfs_cancel_ioend") Also, Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org> > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > fs/iomap/buffered-io.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index ca5c62901541..77d59c159248 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -1421,7 +1421,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc, > if (!count) > folio_end_writeback(folio); > done: > - mapping_set_error(folio->mapping, error); > + mapping_set_error(inode->i_mapping, error); > return error; > } >
On Thu, Sep 29, 2022 at 08:33:06PM +0100, Matthew Wilcox wrote: > On Thu, Sep 29, 2022 at 11:44:49AM -0700, Darrick J. Wong wrote: > > Fixes: e735c0079465 ("iomap: Convert iomap_add_to_ioend() to take a folio") > > Probably-Fixes: 598ecfbaa742 ("iomap: lift the xfs writeback code to iomap") > > I think this is a misuse of Fixes. As I understand it, Fixes: is "Here's > the commit that introduced the bug", not "This is the most recent change > after which this patch will still apply". e735c0079465 only changed > s/page/folio/ in this line of code, so clearly didn't introduce the bug. > > Any kernel containing 598ecfbaa742 has this same bug, so that should be > the Fixes: line. As you say though, 598ecfbaa742 merely moved the code > from xfs_writepage_map(). bfce7d2e2d5e moved it from xfs_do_writepage(), > but 150d5be09ce4 introduced it. Six years ago! Good find. So how about: > > Fixes: 150d5be09ce4 ("xfs: remove xfs_cancel_ioend") Sounds fine to me, though if I hear complaints from AUTOSEL about how the patch does not directly apply to old kernels, I'll forward them to you, because that's what I do now to avoid getting even /more/ email. > Also, > > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org> However, thank you for the quick review. :) --D > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > fs/iomap/buffered-io.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > > index ca5c62901541..77d59c159248 100644 > > --- a/fs/iomap/buffered-io.c > > +++ b/fs/iomap/buffered-io.c > > @@ -1421,7 +1421,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc, > > if (!count) > > folio_end_writeback(folio); > > done: > > - mapping_set_error(folio->mapping, error); > > + mapping_set_error(inode->i_mapping, error); > > return error; > > } > >
On Thu, Sep 29, 2022 at 02:27:17PM -0700, Darrick J. Wong wrote: > On Thu, Sep 29, 2022 at 08:33:06PM +0100, Matthew Wilcox wrote: > > On Thu, Sep 29, 2022 at 11:44:49AM -0700, Darrick J. Wong wrote: > > > Fixes: e735c0079465 ("iomap: Convert iomap_add_to_ioend() to take a folio") > > > Probably-Fixes: 598ecfbaa742 ("iomap: lift the xfs writeback code to iomap") > > > > I think this is a misuse of Fixes. As I understand it, Fixes: is "Here's > > the commit that introduced the bug", not "This is the most recent change > > after which this patch will still apply". e735c0079465 only changed > > s/page/folio/ in this line of code, so clearly didn't introduce the bug. > > > > Any kernel containing 598ecfbaa742 has this same bug, so that should be > > the Fixes: line. As you say though, 598ecfbaa742 merely moved the code > > from xfs_writepage_map(). bfce7d2e2d5e moved it from xfs_do_writepage(), > > but 150d5be09ce4 introduced it. Six years ago! Good find. So how about: > > > > Fixes: 150d5be09ce4 ("xfs: remove xfs_cancel_ioend") > > Sounds fine to me, though if I hear complaints from AUTOSEL about how > the patch does not directly apply to old kernels, I'll forward them to > you, because that's what I do now to avoid getting even /more/ email. Well, autosel is right to complain ... there's a fix which doesn't apply magically to every still-supported kernel containing the bug. I'm happy to own backporting this one as far as necessary. > > Also, > > > > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > However, thank you for the quick review. :) > > --D > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > > --- > > > fs/iomap/buffered-io.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > > > index ca5c62901541..77d59c159248 100644 > > > --- a/fs/iomap/buffered-io.c > > > +++ b/fs/iomap/buffered-io.c > > > @@ -1421,7 +1421,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc, > > > if (!count) > > > folio_end_writeback(folio); > > > done: > > > - mapping_set_error(folio->mapping, error); > > > + mapping_set_error(inode->i_mapping, error); > > > return error; > > > } > > >
On Thu, Sep 29, 2022 at 11:44:49AM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Every now and then I see this crash on arm64: .... > This crash is a result of iomap_writepage_map encountering some sort of > error during writeback and wanting to set that error code in the file > mapping so that fsync will report it. Unfortunately, the code > dereferences folio->mapping after unlocking the folio, which means that > another thread could have removed the page from the page cache > (writeback doesn't hold the invalidation lock) and give it to somebody > else. > > At best we crash the system like above; at worst, we corrupt memory or > set an error on some other unsuspecting file while failing to record the > problems with *this* file. Regardless, fix the problem by reporting the > error to the inode mapping. > > NOTE: Commit 598ecfbaa742 lifted the XFS writeback code to iomap, so > this fix should be backported to XFS in the 4.6-5.4 kernels in addition > to iomap in the 5.5-5.19 kernels. > > Fixes: e735c0079465 ("iomap: Convert iomap_add_to_ioend() to take a folio") > Probably-Fixes: 598ecfbaa742 ("iomap: lift the xfs writeback code to iomap") > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > fs/iomap/buffered-io.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index ca5c62901541..77d59c159248 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -1421,7 +1421,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc, > if (!count) > folio_end_writeback(folio); > done: > - mapping_set_error(folio->mapping, error); > + mapping_set_error(inode->i_mapping, error); > return error; Looks good, though it might be worth putting a comment somewhere in this code to indicate that it's not safe to trust the folio contents after we've submitted the bio, unlocked it without setting writeback, or ended writeback on an unlocked folio... Regardless, Reviewed-by: Dave Chinner <dchinner@redhat.com>
On Thu, Sep 29, 2022 at 10:29:55PM +0100, Matthew Wilcox wrote: > On Thu, Sep 29, 2022 at 02:27:17PM -0700, Darrick J. Wong wrote: > > On Thu, Sep 29, 2022 at 08:33:06PM +0100, Matthew Wilcox wrote: > > > On Thu, Sep 29, 2022 at 11:44:49AM -0700, Darrick J. Wong wrote: > > > > Fixes: e735c0079465 ("iomap: Convert iomap_add_to_ioend() to take a folio") > > > > Probably-Fixes: 598ecfbaa742 ("iomap: lift the xfs writeback code to iomap") > > > > > > I think this is a misuse of Fixes. As I understand it, Fixes: is "Here's > > > the commit that introduced the bug", not "This is the most recent change > > > after which this patch will still apply". e735c0079465 only changed > > > s/page/folio/ in this line of code, so clearly didn't introduce the bug. > > > > > > Any kernel containing 598ecfbaa742 has this same bug, so that should be > > > the Fixes: line. As you say though, 598ecfbaa742 merely moved the code > > > from xfs_writepage_map(). bfce7d2e2d5e moved it from xfs_do_writepage(), > > > but 150d5be09ce4 introduced it. Six years ago! Good find. So how about: > > > > > > Fixes: 150d5be09ce4 ("xfs: remove xfs_cancel_ioend") > > > > Sounds fine to me, though if I hear complaints from AUTOSEL about how > > the patch does not directly apply to old kernels, I'll forward them to > > you, because that's what I do now to avoid getting even /more/ email. > > Well, autosel is right to complain ... there's a fix which doesn't > apply magically to every still-supported kernel containing the bug. > I'm happy to own backporting this one as far as necessary. Are you sure? You're volunteering yourself for 7 LTS backports 5.19, 5.15, 5.10, 5.4, 4.19, 4.14, and 4.9. --D > > > Also, > > > > > > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > > > However, thank you for the quick review. :) > > > > --D > > > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > > > --- > > > > fs/iomap/buffered-io.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > > > > index ca5c62901541..77d59c159248 100644 > > > > --- a/fs/iomap/buffered-io.c > > > > +++ b/fs/iomap/buffered-io.c > > > > @@ -1421,7 +1421,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc, > > > > if (!count) > > > > folio_end_writeback(folio); > > > > done: > > > > - mapping_set_error(folio->mapping, error); > > > > + mapping_set_error(inode->i_mapping, error); > > > > return error; > > > > } > > > >
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index ca5c62901541..77d59c159248 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -1421,7 +1421,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc, if (!count) folio_end_writeback(folio); done: - mapping_set_error(folio->mapping, error); + mapping_set_error(inode->i_mapping, error); return error; }