[PATCH 1/2] xfs: fix a use after free in xfs_defer_finish_recovery

[PATCH 1/2] xfs: fix a use after free in xfs_defer_finish_recovery

[Christoph Hellwig]

dfp will be freed by ->recover_work and thus the tracepoint in case
of an error can lead to a use after free.

Store the defer ops in a local variable to avoid that.

Fixes: 7f2f7531e0d4 ("xfs: store an ops pointer in struct xfs_defer_pending")
Reported-by: kernel test robot <oliver.sang@intel.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_defer.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index ca7f0ac0489604..785c92d2acaa73 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -915,12 +915,13 @@ xfs_defer_finish_recovery(
 	struct xfs_defer_pending	*dfp,
 	struct list_head		*capture_list)
 {
+	const struct xfs_defer_op_type	*ops = dfp->dfp_ops;
 	int				error;
 
-	error = dfp->dfp_ops->recover_work(dfp, capture_list);
+	error = ops->recover_work(dfp, capture_list);
 	if (error)
 		trace_xlog_intent_recovery_failed(mp, error,
-				dfp->dfp_ops->recover_work);
+				ops->recover_work);
 	return error;
 }
 
-- 
2.39.2

[PATCH 2/2] xfs: use the op name in trace_xlog_intent_recovery_failed

[Christoph Hellwig]

Instead of tracing the address of the recovery handler, use the name
in the defer op, similar to other defer ops related tracepoints.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_defer.c |  3 +--
 fs/xfs/xfs_trace.h        | 14 ++++++++------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index 785c92d2acaa73..e99d7890e614e1 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -920,8 +920,7 @@ xfs_defer_finish_recovery(
 
 	error = ops->recover_work(dfp, capture_list);
 	if (error)
-		trace_xlog_intent_recovery_failed(mp, error,
-				ops->recover_work);
+		trace_xlog_intent_recovery_failed(mp, ops, error);
 	return error;
 }
 
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 0efcdb79d10e51..a986c52ff466bc 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -145,21 +145,23 @@ DEFINE_ATTR_LIST_EVENT(xfs_attr_leaf_list);
 DEFINE_ATTR_LIST_EVENT(xfs_attr_node_list);
 
 TRACE_EVENT(xlog_intent_recovery_failed,
-	TP_PROTO(struct xfs_mount *mp, int error, void *function),
-	TP_ARGS(mp, error, function),
+	TP_PROTO(struct xfs_mount *mp, const struct xfs_defer_op_type *ops,
+		 int error),
+	TP_ARGS(mp, ops, error),
 	TP_STRUCT__entry(
 		__field(dev_t, dev)
+		__string(name, ops->name)
 		__field(int, error)
-		__field(void *, function)
 	),
 	TP_fast_assign(
 		__entry->dev = mp->m_super->s_dev;
+		__assign_str(name, ops->name);
 		__entry->error = error;
-		__entry->function = function;
 	),
-	TP_printk("dev %d:%d error %d function %pS",
+	TP_printk("dev %d:%d optype %s error %d",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
-		  __entry->error, __entry->function)
+		  __get_str(name),
+		  __entry->error)
 );
 
 DECLARE_EVENT_CLASS(xfs_perag_class,
-- 
2.39.2

Re: [PATCH 1/2] xfs: fix a use after free in xfs_defer_finish_recovery

[Darrick J. Wong]

On Thu, Dec 28, 2023 at 06:18:29AM +0000, Christoph Hellwig wrote:
> dfp will be freed by ->recover_work and thus the tracepoint in case
> of an error can lead to a use after free.
> 
> Store the defer ops in a local variable to avoid that.
> 
> Fixes: 7f2f7531e0d4 ("xfs: store an ops pointer in struct xfs_defer_pending")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_defer.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index ca7f0ac0489604..785c92d2acaa73 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -915,12 +915,13 @@ xfs_defer_finish_recovery(
>  	struct xfs_defer_pending	*dfp,
>  	struct list_head		*capture_list)
>  {
> +	const struct xfs_defer_op_type	*ops = dfp->dfp_ops;
>  	int				error;
>  
> -	error = dfp->dfp_ops->recover_work(dfp, capture_list);
> +	error = ops->recover_work(dfp, capture_list);

Since I suck at remembering that dfp can be freed by recovery work, can
you add a comment to that effect? e.g.

	/* dfp is freed by recover_work and must not be accessed further */
	error = ops->recover_work(dfp, capture_list);

With that added,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D


>  	if (error)
>  		trace_xlog_intent_recovery_failed(mp, error,
> -				dfp->dfp_ops->recover_work);
> +				ops->recover_work);
>  	return error;
>  }
>  
> -- 
> 2.39.2
> 
> 

Re: [PATCH 2/2] xfs: use the op name in trace_xlog_intent_recovery_failed

[Darrick J. Wong]

On Thu, Dec 28, 2023 at 06:18:30AM +0000, Christoph Hellwig wrote:
> Instead of tracing the address of the recovery handler, use the name
> in the defer op, similar to other defer ops related tracepoints.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Yay fewer pointer shenanigans,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_defer.c |  3 +--
>  fs/xfs/xfs_trace.h        | 14 ++++++++------
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index 785c92d2acaa73..e99d7890e614e1 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -920,8 +920,7 @@ xfs_defer_finish_recovery(
>  
>  	error = ops->recover_work(dfp, capture_list);
>  	if (error)
> -		trace_xlog_intent_recovery_failed(mp, error,
> -				ops->recover_work);
> +		trace_xlog_intent_recovery_failed(mp, ops, error);
>  	return error;
>  }
>  
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 0efcdb79d10e51..a986c52ff466bc 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -145,21 +145,23 @@ DEFINE_ATTR_LIST_EVENT(xfs_attr_leaf_list);
>  DEFINE_ATTR_LIST_EVENT(xfs_attr_node_list);
>  
>  TRACE_EVENT(xlog_intent_recovery_failed,
> -	TP_PROTO(struct xfs_mount *mp, int error, void *function),
> -	TP_ARGS(mp, error, function),
> +	TP_PROTO(struct xfs_mount *mp, const struct xfs_defer_op_type *ops,
> +		 int error),
> +	TP_ARGS(mp, ops, error),
>  	TP_STRUCT__entry(
>  		__field(dev_t, dev)
> +		__string(name, ops->name)
>  		__field(int, error)
> -		__field(void *, function)
>  	),
>  	TP_fast_assign(
>  		__entry->dev = mp->m_super->s_dev;
> +		__assign_str(name, ops->name);
>  		__entry->error = error;
> -		__entry->function = function;
>  	),
> -	TP_printk("dev %d:%d error %d function %pS",
> +	TP_printk("dev %d:%d optype %s error %d",
>  		  MAJOR(__entry->dev), MINOR(__entry->dev),
> -		  __entry->error, __entry->function)
> +		  __get_str(name),
> +		  __entry->error)
>  );
>  
>  DECLARE_EVENT_CLASS(xfs_perag_class,
> -- 
> 2.39.2
> 
> 

Re: [PATCH 1/2] xfs: fix a use after free in xfs_defer_finish_recovery

[Christoph Hellwig]

On Wed, Dec 27, 2023 at 10:26:33PM -0800, Darrick J. Wong wrote:
> > -	error = dfp->dfp_ops->recover_work(dfp, capture_list);
> > +	error = ops->recover_work(dfp, capture_list);
> 
> Since I suck at remembering that dfp can be freed by recovery work, can
> you add a comment to that effect? e.g.

Sure.