* [PATCH 1/3] xfs: don't irele after failing to iget in xfs_attri_recover_work
@ 2026-03-23 21:01 Darrick J. Wong
2026-03-23 21:03 ` [PATCH 2/3] xfs: don't expose stale pointers to callers of xlog_recover_iget Darrick J. Wong
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: Darrick J. Wong @ 2026-03-23 21:01 UTC (permalink / raw)
To: cem; +Cc: Long Li, linux-xfs, hch
From: Darrick J. Wong <djwong@kernel.org>
xlog_recovery_iget* never set @ip to a valid pointer if they return
zero, so this irele will walk off a dangling pointer. Fix that.
Cc: <stable@vger.kernel.org> # v6.10
Fixes: ae673f534a3097 ("xfs: record inode generation in xattr update log intent items")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Long Li <leo.lilong@huawei.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_attr_item.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 354472bf45f145..fe909bc44c3a79 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -653,7 +653,6 @@ xfs_attri_recover_work(
break;
}
if (error) {
- xfs_irele(ip);
XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, attrp,
sizeof(*attrp));
return ERR_PTR(-EFSCORRUPTED);
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/3] xfs: don't expose stale pointers to callers of xlog_recover_iget
2026-03-23 21:01 [PATCH 1/3] xfs: don't irele after failing to iget in xfs_attri_recover_work Darrick J. Wong
@ 2026-03-23 21:03 ` Darrick J. Wong
2026-03-24 6:17 ` Christoph Hellwig
2026-03-26 13:04 ` Carlos Maiolino
2026-03-23 21:04 ` [PATCH 3/3] xfs: remove file_path tracepoint data Darrick J. Wong
` (3 subsequent siblings)
4 siblings, 2 replies; 14+ messages in thread
From: Darrick J. Wong @ 2026-03-23 21:03 UTC (permalink / raw)
To: cem; +Cc: Long Li, linux-xfs, hch
From: Darrick J. Wong <djwong@kernel.org>
Fix this function to avoid exposing a stale pointer to the caller when
returning an error code after dqattach fails.
Cc: <stable@vger.kernel.org> # v5.15
Fixes: 4bc619833f738f ("xfs: refactor xfs_iget calls from log intent recovery")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Long Li <leo.lilong@huawei.com>
---
fs/xfs/xfs_log_recover.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 09e6678ca4878e..0e91a62348eb79 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1745,21 +1745,23 @@ xlog_recover_iget(
xfs_ino_t ino,
struct xfs_inode **ipp)
{
+ struct xfs_inode *ip;
int error;
- error = xfs_iget(mp, NULL, ino, 0, 0, ipp);
+ error = xfs_iget(mp, NULL, ino, 0, 0, &ip);
if (error)
return error;
- error = xfs_qm_dqattach(*ipp);
+ error = xfs_qm_dqattach(ip);
if (error) {
- xfs_irele(*ipp);
+ xfs_irele(ip);
return error;
}
- if (VFS_I(*ipp)->i_nlink == 0)
- xfs_iflags_set(*ipp, XFS_IRECOVERY);
+ if (VFS_I(ip)->i_nlink == 0)
+ xfs_iflags_set(ip, XFS_IRECOVERY);
+ *ipp = ip;
return 0;
}
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/3] xfs: remove file_path tracepoint data
2026-03-23 21:01 [PATCH 1/3] xfs: don't irele after failing to iget in xfs_attri_recover_work Darrick J. Wong
2026-03-23 21:03 ` [PATCH 2/3] xfs: don't expose stale pointers to callers of xlog_recover_iget Darrick J. Wong
@ 2026-03-23 21:04 ` Darrick J. Wong
2026-03-24 6:18 ` Christoph Hellwig
2026-03-26 12:31 ` Carlos Maiolino
2026-03-26 12:28 ` [PATCH 1/3] xfs: don't irele after failing to iget in xfs_attri_recover_work Carlos Maiolino
` (2 subsequent siblings)
4 siblings, 2 replies; 14+ messages in thread
From: Darrick J. Wong @ 2026-03-23 21:04 UTC (permalink / raw)
To: cem; +Cc: linux-xfs, hch, david.laight.linux, rostedt
From: Darrick J. Wong <djwong@kernel.org>
The xfile/xmbuf shmem file descriptions are no longer as detailed as
they were when online fsck was first merged, because moving to static
strings in commit 60382993a2e180 ("xfs: get rid of the
xchk_xfile_*_descr calls") removed a memory allocation and hence a
source of failure.
However this makes encoding the description in the tracepoints sort of a
waste of memory. David Laight also points out that file_path doesn't
zero the whole buffer which causes exposure of stale trace bytes, and
Steven Rostedt wonders why we're not using a dynamic array for the file
path.
I don't think this is worth fixing, so let's just rip it out.
Cc: rostedt@goodmis.org
Cc: david.laight.linux@gmail.com
Link: https://lore.kernel.org/linux-xfs/20260323172204.work.979-kees@kernel.org/
Cc: <stable@vger.kernel.org> # v6.11
Fixes: 19ebc8f84ea12e ("xfs: fix file_path handling in tracepoints")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
fs/xfs/scrub/trace.h | 12 ++----------
fs/xfs/xfs_trace.h | 11 ++---------
2 files changed, 4 insertions(+), 19 deletions(-)
diff --git a/fs/xfs/scrub/trace.h b/fs/xfs/scrub/trace.h
index 39ea651cbb7510..286c5f5e054449 100644
--- a/fs/xfs/scrub/trace.h
+++ b/fs/xfs/scrub/trace.h
@@ -972,20 +972,12 @@ TRACE_EVENT(xfile_create,
TP_STRUCT__entry(
__field(dev_t, dev)
__field(unsigned long, ino)
- __array(char, pathname, MAXNAMELEN)
),
TP_fast_assign(
- char *path;
-
__entry->ino = file_inode(xf->file)->i_ino;
- path = file_path(xf->file, __entry->pathname, MAXNAMELEN);
- if (IS_ERR(path))
- strncpy(__entry->pathname, "(unknown)",
- sizeof(__entry->pathname));
),
- TP_printk("xfino 0x%lx path '%s'",
- __entry->ino,
- __entry->pathname)
+ TP_printk("xfino 0x%lx",
+ __entry->ino)
);
TRACE_EVENT(xfile_destroy,
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 813e5a9f57eb7a..91c4c9422c8739 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -5091,23 +5091,16 @@ TRACE_EVENT(xmbuf_create,
TP_STRUCT__entry(
__field(dev_t, dev)
__field(unsigned long, ino)
- __array(char, pathname, MAXNAMELEN)
),
TP_fast_assign(
- char *path;
struct file *file = btp->bt_file;
__entry->dev = btp->bt_mount->m_super->s_dev;
__entry->ino = file_inode(file)->i_ino;
- path = file_path(file, __entry->pathname, MAXNAMELEN);
- if (IS_ERR(path))
- strncpy(__entry->pathname, "(unknown)",
- sizeof(__entry->pathname));
),
- TP_printk("dev %d:%d xmino 0x%lx path '%s'",
+ TP_printk("dev %d:%d xmino 0x%lx",
MAJOR(__entry->dev), MINOR(__entry->dev),
- __entry->ino,
- __entry->pathname)
+ __entry->ino)
);
TRACE_EVENT(xmbuf_free,
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] xfs: don't expose stale pointers to callers of xlog_recover_iget
2026-03-23 21:03 ` [PATCH 2/3] xfs: don't expose stale pointers to callers of xlog_recover_iget Darrick J. Wong
@ 2026-03-24 6:17 ` Christoph Hellwig
2026-03-24 17:15 ` Darrick J. Wong
2026-03-26 13:04 ` Carlos Maiolino
1 sibling, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2026-03-24 6:17 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, Long Li, linux-xfs, hch
On Mon, Mar 23, 2026 at 02:03:51PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Fix this function to avoid exposing a stale pointer to the caller when
> returning an error code after dqattach fails.
I still think this is silly.
> Cc: <stable@vger.kernel.org> # v5.15
> Fixes: 4bc619833f738f ("xfs: refactor xfs_iget calls from log intent recovery")
.. and certainly does not actually fix anything.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] xfs: remove file_path tracepoint data
2026-03-23 21:04 ` [PATCH 3/3] xfs: remove file_path tracepoint data Darrick J. Wong
@ 2026-03-24 6:18 ` Christoph Hellwig
2026-03-26 12:31 ` Carlos Maiolino
1 sibling, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2026-03-24 6:18 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: cem, linux-xfs, hch, david.laight.linux, rostedt
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] xfs: don't expose stale pointers to callers of xlog_recover_iget
2026-03-24 6:17 ` Christoph Hellwig
@ 2026-03-24 17:15 ` Darrick J. Wong
2026-03-25 5:44 ` Christoph Hellwig
0 siblings, 1 reply; 14+ messages in thread
From: Darrick J. Wong @ 2026-03-24 17:15 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: cem, Long Li, linux-xfs, hch
On Mon, Mar 23, 2026 at 11:17:51PM -0700, Christoph Hellwig wrote:
> On Mon, Mar 23, 2026 at 02:03:51PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Fix this function to avoid exposing a stale pointer to the caller when
> > returning an error code after dqattach fails.
>
> I still think this is silly.
>
> > Cc: <stable@vger.kernel.org> # v5.15
> > Fixes: 4bc619833f738f ("xfs: refactor xfs_iget calls from log intent recovery")
>
> .. and certainly does not actually fix anything.
<nod> I'll let cem decide if he wants to trade more stack usage for
removing theoretical bugs via this patch, then.
--D
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] xfs: don't expose stale pointers to callers of xlog_recover_iget
2026-03-24 17:15 ` Darrick J. Wong
@ 2026-03-25 5:44 ` Christoph Hellwig
0 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2026-03-25 5:44 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Christoph Hellwig, cem, Long Li, linux-xfs, hch
On Tue, Mar 24, 2026 at 10:15:13AM -0700, Darrick J. Wong wrote:
> > I still think this is silly.
> >
> > > Cc: <stable@vger.kernel.org> # v5.15
> > > Fixes: 4bc619833f738f ("xfs: refactor xfs_iget calls from log intent recovery")
> >
> > .. and certainly does not actually fix anything.
>
> <nod> I'll let cem decide if he wants to trade more stack usage for
> removing theoretical bugs via this patch, then.
I don't think it even removes a theoretical bug. It papers over callers
not understanding the calling convention assuming they also have a NULL
check which they'd otherwise don't need. That's the kind of cargo cult
programming that makes it really hard to reason about the rules.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] xfs: don't irele after failing to iget in xfs_attri_recover_work
2026-03-23 21:01 [PATCH 1/3] xfs: don't irele after failing to iget in xfs_attri_recover_work Darrick J. Wong
2026-03-23 21:03 ` [PATCH 2/3] xfs: don't expose stale pointers to callers of xlog_recover_iget Darrick J. Wong
2026-03-23 21:04 ` [PATCH 3/3] xfs: remove file_path tracepoint data Darrick J. Wong
@ 2026-03-26 12:28 ` Carlos Maiolino
2026-03-26 14:52 ` Darrick J. Wong
2026-03-26 12:32 ` Carlos Maiolino
2026-03-26 17:11 ` Carlos Maiolino
4 siblings, 1 reply; 14+ messages in thread
From: Carlos Maiolino @ 2026-03-26 12:28 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Long Li, linux-xfs, hch
On Mon, Mar 23, 2026 at 02:01:57PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> xlog_recovery_iget* never set @ip to a valid pointer if they return
> zero,
* if they return an error * ?
It will always return zero when setting @ip.
I can update it at commit time if you're ok with it.
>so this irele will walk off a dangling pointer. Fix that.
>
> Cc: <stable@vger.kernel.org> # v6.10
> Fixes: ae673f534a3097 ("xfs: record inode generation in xattr update log intent items")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Long Li <leo.lilong@huawei.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/xfs_attr_item.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 354472bf45f145..fe909bc44c3a79 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -653,7 +653,6 @@ xfs_attri_recover_work(
> break;
> }
> if (error) {
> - xfs_irele(ip);
> XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, attrp,
> sizeof(*attrp));
> return ERR_PTR(-EFSCORRUPTED);
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] xfs: remove file_path tracepoint data
2026-03-23 21:04 ` [PATCH 3/3] xfs: remove file_path tracepoint data Darrick J. Wong
2026-03-24 6:18 ` Christoph Hellwig
@ 2026-03-26 12:31 ` Carlos Maiolino
1 sibling, 0 replies; 14+ messages in thread
From: Carlos Maiolino @ 2026-03-26 12:31 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch, david.laight.linux, rostedt
On Mon, Mar 23, 2026 at 02:04:33PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> The xfile/xmbuf shmem file descriptions are no longer as detailed as
> they were when online fsck was first merged, because moving to static
> strings in commit 60382993a2e180 ("xfs: get rid of the
> xchk_xfile_*_descr calls") removed a memory allocation and hence a
> source of failure.
>
> However this makes encoding the description in the tracepoints sort of a
> waste of memory. David Laight also points out that file_path doesn't
> zero the whole buffer which causes exposure of stale trace bytes, and
> Steven Rostedt wonders why we're not using a dynamic array for the file
> path.
>
> I don't think this is worth fixing, so let's just rip it out.
>
> Cc: rostedt@goodmis.org
> Cc: david.laight.linux@gmail.com
> Link: https://lore.kernel.org/linux-xfs/20260323172204.work.979-kees@kernel.org/
> Cc: <stable@vger.kernel.org> # v6.11
> Fixes: 19ebc8f84ea12e ("xfs: fix file_path handling in tracepoints")
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> fs/xfs/scrub/trace.h | 12 ++----------
> fs/xfs/xfs_trace.h | 11 ++---------
> 2 files changed, 4 insertions(+), 19 deletions(-)
>
> diff --git a/fs/xfs/scrub/trace.h b/fs/xfs/scrub/trace.h
> index 39ea651cbb7510..286c5f5e054449 100644
> --- a/fs/xfs/scrub/trace.h
> +++ b/fs/xfs/scrub/trace.h
> @@ -972,20 +972,12 @@ TRACE_EVENT(xfile_create,
> TP_STRUCT__entry(
> __field(dev_t, dev)
> __field(unsigned long, ino)
> - __array(char, pathname, MAXNAMELEN)
> ),
> TP_fast_assign(
> - char *path;
> -
> __entry->ino = file_inode(xf->file)->i_ino;
> - path = file_path(xf->file, __entry->pathname, MAXNAMELEN);
> - if (IS_ERR(path))
> - strncpy(__entry->pathname, "(unknown)",
> - sizeof(__entry->pathname));
> ),
> - TP_printk("xfino 0x%lx path '%s'",
> - __entry->ino,
> - __entry->pathname)
> + TP_printk("xfino 0x%lx",
> + __entry->ino)
> );
>
> TRACE_EVENT(xfile_destroy,
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 813e5a9f57eb7a..91c4c9422c8739 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -5091,23 +5091,16 @@ TRACE_EVENT(xmbuf_create,
> TP_STRUCT__entry(
> __field(dev_t, dev)
> __field(unsigned long, ino)
> - __array(char, pathname, MAXNAMELEN)
> ),
> TP_fast_assign(
> - char *path;
> struct file *file = btp->bt_file;
>
> __entry->dev = btp->bt_mount->m_super->s_dev;
> __entry->ino = file_inode(file)->i_ino;
> - path = file_path(file, __entry->pathname, MAXNAMELEN);
> - if (IS_ERR(path))
> - strncpy(__entry->pathname, "(unknown)",
> - sizeof(__entry->pathname));
> ),
> - TP_printk("dev %d:%d xmino 0x%lx path '%s'",
> + TP_printk("dev %d:%d xmino 0x%lx",
> MAJOR(__entry->dev), MINOR(__entry->dev),
> - __entry->ino,
> - __entry->pathname)
> + __entry->ino)
> );
>
> TRACE_EVENT(xmbuf_free,
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] xfs: don't irele after failing to iget in xfs_attri_recover_work
2026-03-23 21:01 [PATCH 1/3] xfs: don't irele after failing to iget in xfs_attri_recover_work Darrick J. Wong
` (2 preceding siblings ...)
2026-03-26 12:28 ` [PATCH 1/3] xfs: don't irele after failing to iget in xfs_attri_recover_work Carlos Maiolino
@ 2026-03-26 12:32 ` Carlos Maiolino
2026-03-26 17:11 ` Carlos Maiolino
4 siblings, 0 replies; 14+ messages in thread
From: Carlos Maiolino @ 2026-03-26 12:32 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Long Li, linux-xfs, hch
On Mon, Mar 23, 2026 at 02:01:57PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> xlog_recovery_iget* never set @ip to a valid pointer if they return
> zero, so this irele will walk off a dangling pointer. Fix that.
Oh, yeah, and with that fixed...
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
>
> Cc: <stable@vger.kernel.org> # v6.10
> Fixes: ae673f534a3097 ("xfs: record inode generation in xattr update log intent items")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Long Li <leo.lilong@huawei.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/xfs_attr_item.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 354472bf45f145..fe909bc44c3a79 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -653,7 +653,6 @@ xfs_attri_recover_work(
> break;
> }
> if (error) {
> - xfs_irele(ip);
> XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, attrp,
> sizeof(*attrp));
> return ERR_PTR(-EFSCORRUPTED);
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] xfs: don't expose stale pointers to callers of xlog_recover_iget
2026-03-23 21:03 ` [PATCH 2/3] xfs: don't expose stale pointers to callers of xlog_recover_iget Darrick J. Wong
2026-03-24 6:17 ` Christoph Hellwig
@ 2026-03-26 13:04 ` Carlos Maiolino
2026-03-26 14:54 ` Darrick J. Wong
1 sibling, 1 reply; 14+ messages in thread
From: Carlos Maiolino @ 2026-03-26 13:04 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Long Li, linux-xfs, hch
On Mon, Mar 23, 2026 at 02:03:51PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Fix this function to avoid exposing a stale pointer to the caller when
> returning an error code after dqattach fails.
>
> Cc: <stable@vger.kernel.org> # v5.15
> Fixes: 4bc619833f738f ("xfs: refactor xfs_iget calls from log intent recovery")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Long Li <leo.lilong@huawei.com>
> ---
Hi.
> fs/xfs/xfs_log_recover.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 09e6678ca4878e..0e91a62348eb79 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -1745,21 +1745,23 @@ xlog_recover_iget(
> xfs_ino_t ino,
> struct xfs_inode **ipp)
> {
> + struct xfs_inode *ip;
> int error;
>
> - error = xfs_iget(mp, NULL, ino, 0, 0, ipp);
> + error = xfs_iget(mp, NULL, ino, 0, 0, &ip);
> if (error)
> return error;
>
> - error = xfs_qm_dqattach(*ipp);
> + error = xfs_qm_dqattach(ip);
> if (error) {
> - xfs_irele(*ipp);
> + xfs_irele(ip);
> return error;
> }
>
> - if (VFS_I(*ipp)->i_nlink == 0)
> - xfs_iflags_set(*ipp, XFS_IRECOVERY);
> + if (VFS_I(ip)->i_nlink == 0)
> + xfs_iflags_set(ip, XFS_IRECOVERY);
>
> + *ipp = ip;
> return 0;
Honestly I tend to agree with Christoph here. I don't really understand
what bug, real or theoretical, this is trying to fix, other than
somebody not aborting whatever operation is being done if an error
is returned.
I don't see any issue with an extra inode pointer being passed on the
stack here, but I don't see any benefit for this either.
Did you fall into this somehow? Have you seen this into the wild? I'll
pull the other patches meanwhile, but I think this one deserves more
discussion.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] xfs: don't irele after failing to iget in xfs_attri_recover_work
2026-03-26 12:28 ` [PATCH 1/3] xfs: don't irele after failing to iget in xfs_attri_recover_work Carlos Maiolino
@ 2026-03-26 14:52 ` Darrick J. Wong
0 siblings, 0 replies; 14+ messages in thread
From: Darrick J. Wong @ 2026-03-26 14:52 UTC (permalink / raw)
To: Carlos Maiolino; +Cc: Long Li, linux-xfs, hch
On Thu, Mar 26, 2026 at 01:28:20PM +0100, Carlos Maiolino wrote:
> On Mon, Mar 23, 2026 at 02:01:57PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > xlog_recovery_iget* never set @ip to a valid pointer if they return
> > zero,
>
> * if they return an error * ?
> It will always return zero when setting @ip.
>
> I can update it at commit time if you're ok with it.
Er, yes, "...if they return an error" is correct.
--D
> >so this irele will walk off a dangling pointer. Fix that.
> >
> > Cc: <stable@vger.kernel.org> # v6.10
> > Fixes: ae673f534a3097 ("xfs: record inode generation in xattr update log intent items")
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > Reviewed-by: Long Li <leo.lilong@huawei.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > ---
> > fs/xfs/xfs_attr_item.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> > index 354472bf45f145..fe909bc44c3a79 100644
> > --- a/fs/xfs/xfs_attr_item.c
> > +++ b/fs/xfs/xfs_attr_item.c
> > @@ -653,7 +653,6 @@ xfs_attri_recover_work(
> > break;
> > }
> > if (error) {
> > - xfs_irele(ip);
> > XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, attrp,
> > sizeof(*attrp));
> > return ERR_PTR(-EFSCORRUPTED);
> >
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] xfs: don't expose stale pointers to callers of xlog_recover_iget
2026-03-26 13:04 ` Carlos Maiolino
@ 2026-03-26 14:54 ` Darrick J. Wong
0 siblings, 0 replies; 14+ messages in thread
From: Darrick J. Wong @ 2026-03-26 14:54 UTC (permalink / raw)
To: Carlos Maiolino; +Cc: Long Li, linux-xfs, hch
On Thu, Mar 26, 2026 at 02:04:19PM +0100, Carlos Maiolino wrote:
> On Mon, Mar 23, 2026 at 02:03:51PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Fix this function to avoid exposing a stale pointer to the caller when
> > returning an error code after dqattach fails.
> >
> > Cc: <stable@vger.kernel.org> # v5.15
> > Fixes: 4bc619833f738f ("xfs: refactor xfs_iget calls from log intent recovery")
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > Reviewed-by: Long Li <leo.lilong@huawei.com>
> > ---
>
> Hi.
>
>
> > fs/xfs/xfs_log_recover.c | 12 +++++++-----
> > 1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index 09e6678ca4878e..0e91a62348eb79 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -1745,21 +1745,23 @@ xlog_recover_iget(
> > xfs_ino_t ino,
> > struct xfs_inode **ipp)
> > {
> > + struct xfs_inode *ip;
> > int error;
> >
> > - error = xfs_iget(mp, NULL, ino, 0, 0, ipp);
> > + error = xfs_iget(mp, NULL, ino, 0, 0, &ip);
> > if (error)
> > return error;
> >
> > - error = xfs_qm_dqattach(*ipp);
> > + error = xfs_qm_dqattach(ip);
> > if (error) {
> > - xfs_irele(*ipp);
> > + xfs_irele(ip);
> > return error;
> > }
> >
> > - if (VFS_I(*ipp)->i_nlink == 0)
> > - xfs_iflags_set(*ipp, XFS_IRECOVERY);
> > + if (VFS_I(ip)->i_nlink == 0)
> > + xfs_iflags_set(ip, XFS_IRECOVERY);
> >
> > + *ipp = ip;
> > return 0;
>
> Honestly I tend to agree with Christoph here. I don't really understand
> what bug, real or theoretical, this is trying to fix, other than
> somebody not aborting whatever operation is being done if an error
> is returned.
>
> I don't see any issue with an extra inode pointer being passed on the
> stack here, but I don't see any benefit for this either.
>
> Did you fall into this somehow? Have you seen this into the wild? I'll
> pull the other patches meanwhile, but I think this one deserves more
> discussion.
Yeah, look who wrote the function and the bad error handling. You could
say that the *author* didn't understand his own function's outparam
conventions and wants to prevent himself or anyone else from making
further mistakes. ;)
--D
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] xfs: don't irele after failing to iget in xfs_attri_recover_work
2026-03-23 21:01 [PATCH 1/3] xfs: don't irele after failing to iget in xfs_attri_recover_work Darrick J. Wong
` (3 preceding siblings ...)
2026-03-26 12:32 ` Carlos Maiolino
@ 2026-03-26 17:11 ` Carlos Maiolino
4 siblings, 0 replies; 14+ messages in thread
From: Carlos Maiolino @ 2026-03-26 17:11 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Long Li, linux-xfs, hch
On Mon, 23 Mar 2026 14:01:57 -0700, Darrick J. Wong wrote:
> xlog_recovery_iget* never set @ip to a valid pointer if they return
> zero, so this irele will walk off a dangling pointer. Fix that.
>
>
Applied to for-next, thanks!
[1/3] xfs: don't irele after failing to iget in xfs_attri_recover_work
commit: 70685c291ef82269180758130394ecdc4496b52c
[2/3] xfs: don't expose stale pointers to callers of xlog_recover_iget
(no commit info)
[3/3] xfs: remove file_path tracepoint data
commit: e31c53a8060e134111ed095783fee0aa0c43b080
Best regards,
--
Carlos Maiolino <cem@kernel.org>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2026-03-26 17:11 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-23 21:01 [PATCH 1/3] xfs: don't irele after failing to iget in xfs_attri_recover_work Darrick J. Wong
2026-03-23 21:03 ` [PATCH 2/3] xfs: don't expose stale pointers to callers of xlog_recover_iget Darrick J. Wong
2026-03-24 6:17 ` Christoph Hellwig
2026-03-24 17:15 ` Darrick J. Wong
2026-03-25 5:44 ` Christoph Hellwig
2026-03-26 13:04 ` Carlos Maiolino
2026-03-26 14:54 ` Darrick J. Wong
2026-03-23 21:04 ` [PATCH 3/3] xfs: remove file_path tracepoint data Darrick J. Wong
2026-03-24 6:18 ` Christoph Hellwig
2026-03-26 12:31 ` Carlos Maiolino
2026-03-26 12:28 ` [PATCH 1/3] xfs: don't irele after failing to iget in xfs_attri_recover_work Carlos Maiolino
2026-03-26 14:52 ` Darrick J. Wong
2026-03-26 12:32 ` Carlos Maiolino
2026-03-26 17:11 ` Carlos Maiolino
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox