| Message ID | 20241023133755.524345-2-hch@lst.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [1/2] xfs: fix finding a last resort AG in xfs_filestream_pick_ag | expand |
On Wed, Oct 23, 2024 at 03:37:22PM +0200, Christoph Hellwig wrote: > When the main loop in xfs_filestream_pick_ag fails to find a suitable > AG it tries to just pick the online AG. But the loop for that uses > args->pag as loop iterator while the later code expects pag to be > set. Fix this by reusing the max_pag case for this last resort, and > also add a check for impossible case of no AG just to make sure that > the uninitialized pag doesn't even escape in theory. > > Reported-by: syzbot+4125a3c514e3436a02e6@syzkaller.appspotmail.com > Signed-off-by: Christoph Hellwig <hch@lst.de> > Tested-by: syzbot+4125a3c514e3436a02e6@syzkaller.appspotmail.com Cc: <stable@vger.kernel.org> # v6.3 Fixes: f8f1ed1ab3baba ("xfs: return a referenced perag from filestreams allocator") Perhaps? Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/xfs_filestream.c | 23 ++++++++++++----------- > fs/xfs/xfs_trace.h | 15 +++++---------- > 2 files changed, 17 insertions(+), 21 deletions(-) > > diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c > index e3aaa0555597..88bd23ce74cd 100644 > --- a/fs/xfs/xfs_filestream.c > +++ b/fs/xfs/xfs_filestream.c > @@ -64,7 +64,7 @@ xfs_filestream_pick_ag( > struct xfs_perag *pag; > struct xfs_perag *max_pag = NULL; > xfs_extlen_t minlen = *longest; > - xfs_extlen_t free = 0, minfree, maxfree = 0; > + xfs_extlen_t minfree, maxfree = 0; > xfs_agnumber_t agno; > bool first_pass = true; > int err; > @@ -107,7 +107,6 @@ xfs_filestream_pick_ag( > !(flags & XFS_PICK_USERDATA) || > (flags & XFS_PICK_LOWSPACE))) { > /* Break out, retaining the reference on the AG. */ > - free = pag->pagf_freeblks; > break; > } > } > @@ -150,23 +149,25 @@ xfs_filestream_pick_ag( > * grab. > */ > if (!max_pag) { > - for_each_perag_wrap(args->mp, 0, start_agno, args->pag) > + for_each_perag_wrap(args->mp, 0, start_agno, pag) { > + max_pag = pag; > break; > - atomic_inc(&args->pag->pagf_fstrms); > - *longest = 0; > - } else { > - pag = max_pag; > - free = maxfree; > - atomic_inc(&pag->pagf_fstrms); > + } > + > + /* Bail if there are no AGs at all to select from. */ > + if (!max_pag) > + return -ENOSPC; > } > + > + pag = max_pag; > + atomic_inc(&pag->pagf_fstrms); > } else if (max_pag) { > xfs_perag_rele(max_pag); > } > > - trace_xfs_filestream_pick(pag, pino, free); > + trace_xfs_filestream_pick(pag, pino); > args->pag = pag; > return 0; > - > } > > static struct xfs_inode * > diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h > index ee9f0b1f548d..fcb2bad4f76e 100644 > --- a/fs/xfs/xfs_trace.h > +++ b/fs/xfs/xfs_trace.h > @@ -691,8 +691,8 @@ DEFINE_FILESTREAM_EVENT(xfs_filestream_lookup); > DEFINE_FILESTREAM_EVENT(xfs_filestream_scan); > > TRACE_EVENT(xfs_filestream_pick, > - TP_PROTO(struct xfs_perag *pag, xfs_ino_t ino, xfs_extlen_t free), > - TP_ARGS(pag, ino, free), > + TP_PROTO(struct xfs_perag *pag, xfs_ino_t ino), > + TP_ARGS(pag, ino), > TP_STRUCT__entry( > __field(dev_t, dev) > __field(xfs_ino_t, ino) > @@ -703,14 +703,9 @@ TRACE_EVENT(xfs_filestream_pick, > TP_fast_assign( > __entry->dev = pag->pag_mount->m_super->s_dev; > __entry->ino = ino; > - if (pag) { > - __entry->agno = pag->pag_agno; > - __entry->streams = atomic_read(&pag->pagf_fstrms); > - } else { > - __entry->agno = NULLAGNUMBER; > - __entry->streams = 0; > - } > - __entry->free = free; > + __entry->agno = pag->pag_agno; > + __entry->streams = atomic_read(&pag->pagf_fstrms); > + __entry->free = pag->pagf_freeblks; > ), > TP_printk("dev %d:%d ino 0x%llx agno 0x%x streams %d free %d", > MAJOR(__entry->dev), MINOR(__entry->dev), > -- > 2.45.2 > >
On Wed, 23 Oct 2024 15:37:22 +0200, Christoph Hellwig wrote: > When the main loop in xfs_filestream_pick_ag fails to find a suitable > AG it tries to just pick the online AG. But the loop for that uses > args->pag as loop iterator while the later code expects pag to be > set. Fix this by reusing the max_pag case for this last resort, and > also add a check for impossible case of no AG just to make sure that > the uninitialized pag doesn't even escape in theory. > > [...] Applied to for-next, thanks! [1/2] xfs: fix finding a last resort AG in xfs_filestream_pick_ag commit: dc60992ce76fbc2f71c2674f435ff6bde2108028 [2/2] xfs: streamline xfs_filestream_pick_ag commit: 81a1e1c32ef474c20ccb9f730afe1ac25b1c62a4 Best regards,
diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c index e3aaa0555597..88bd23ce74cd 100644 --- a/fs/xfs/xfs_filestream.c +++ b/fs/xfs/xfs_filestream.c @@ -64,7 +64,7 @@ xfs_filestream_pick_ag( struct xfs_perag *pag; struct xfs_perag *max_pag = NULL; xfs_extlen_t minlen = *longest; - xfs_extlen_t free = 0, minfree, maxfree = 0; + xfs_extlen_t minfree, maxfree = 0; xfs_agnumber_t agno; bool first_pass = true; int err; @@ -107,7 +107,6 @@ xfs_filestream_pick_ag( !(flags & XFS_PICK_USERDATA) || (flags & XFS_PICK_LOWSPACE))) { /* Break out, retaining the reference on the AG. */ - free = pag->pagf_freeblks; break; } } @@ -150,23 +149,25 @@ xfs_filestream_pick_ag( * grab. */ if (!max_pag) { - for_each_perag_wrap(args->mp, 0, start_agno, args->pag) + for_each_perag_wrap(args->mp, 0, start_agno, pag) { + max_pag = pag; break; - atomic_inc(&args->pag->pagf_fstrms); - *longest = 0; - } else { - pag = max_pag; - free = maxfree; - atomic_inc(&pag->pagf_fstrms); + } + + /* Bail if there are no AGs at all to select from. */ + if (!max_pag) + return -ENOSPC; } + + pag = max_pag; + atomic_inc(&pag->pagf_fstrms); } else if (max_pag) { xfs_perag_rele(max_pag); } - trace_xfs_filestream_pick(pag, pino, free); + trace_xfs_filestream_pick(pag, pino); args->pag = pag; return 0; - } static struct xfs_inode * diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h index ee9f0b1f548d..fcb2bad4f76e 100644 --- a/fs/xfs/xfs_trace.h +++ b/fs/xfs/xfs_trace.h @@ -691,8 +691,8 @@ DEFINE_FILESTREAM_EVENT(xfs_filestream_lookup); DEFINE_FILESTREAM_EVENT(xfs_filestream_scan); TRACE_EVENT(xfs_filestream_pick, - TP_PROTO(struct xfs_perag *pag, xfs_ino_t ino, xfs_extlen_t free), - TP_ARGS(pag, ino, free), + TP_PROTO(struct xfs_perag *pag, xfs_ino_t ino), + TP_ARGS(pag, ino), TP_STRUCT__entry( __field(dev_t, dev) __field(xfs_ino_t, ino) @@ -703,14 +703,9 @@ TRACE_EVENT(xfs_filestream_pick, TP_fast_assign( __entry->dev = pag->pag_mount->m_super->s_dev; __entry->ino = ino; - if (pag) { - __entry->agno = pag->pag_agno; - __entry->streams = atomic_read(&pag->pagf_fstrms); - } else { - __entry->agno = NULLAGNUMBER; - __entry->streams = 0; - } - __entry->free = free; + __entry->agno = pag->pag_agno; + __entry->streams = atomic_read(&pag->pagf_fstrms); + __entry->free = pag->pagf_freeblks; ), TP_printk("dev %d:%d ino 0x%llx agno 0x%x streams %d free %d", MAJOR(__entry->dev), MINOR(__entry->dev),