public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
* [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