Re: [PATCH 2/2] Btrfs: get rid of pointless wtag variable in async-thread.c

[Nikolay Borisov <nborisov@suse.com>]

On 13.08.19 г. 10:26 ч., Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Commit ac0c7cf8be00 ("btrfs: fix crash when tracepoint arguments are
> freed by wq callbacks") added a void pointer, wtag, which is passed into
> trace_btrfs_all_work_done() instead of the freed work item. This is
> silly for a few reasons:
> 
> 1. The freed work item still has the same address.
> 2. work is still in scope after it's freed, so assigning wtag doesn't
>    stop anyone from using it.
> 3. The tracepoint has always taken a void * argument, so assigning wtag
>    doesn't actually make things any more type-safe. (Note that the
>    original bug in commit bc074524e123 ("btrfs: prefix fsid to all trace
>    events") was that the void * was implicitly casted when it was passed
>    to btrfs_work_owner() in the trace point itself).
> 
> Instead, let's add some clearer warnings as comments.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com> albeit one small nit
below.

> ---
>  fs/btrfs/async-thread.c      | 21 ++++++++-------------
>  include/trace/events/btrfs.h |  6 +++---
>  2 files changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/btrfs/async-thread.c b/fs/btrfs/async-thread.c
> index d105d3df6fa6..baeb4058e9dc 100644
> --- a/fs/btrfs/async-thread.c
> +++ b/fs/btrfs/async-thread.c
> @@ -226,7 +226,6 @@ static void run_ordered_work(struct btrfs_work *self)
>  	struct btrfs_work *work;
>  	spinlock_t *lock = &wq->list_lock;
>  	unsigned long flags;
> -	void *wtag;
>  	bool free_self = false;
>  
>  	while (1) {
> @@ -281,21 +280,19 @@ static void run_ordered_work(struct btrfs_work *self)
>  		} else {
>  			/*
>  			 * We don't want to call the ordered free functions with
> -			 * the lock held though. Save the work as tag for the
> -			 * trace event, because the callback could free the
> -			 * structure.
> +			 * the lock held.
>  			 */
> -			wtag = work;
>  			work->ordered_free(work);
> -			trace_btrfs_all_work_done(wq->fs_info, wtag);
> +			/* work must not be dereferenced past this point. */

perhaps prefix the comment with NB: to draw attention to that comment.

> +			trace_btrfs_all_work_done(wq->fs_info, work);
>  		}
>  	}
>  	spin_unlock_irqrestore(lock, flags);
>  
>  	if (free_self) {
> -		wtag = self;
>  		self->ordered_free(self);
> -		trace_btrfs_all_work_done(wq->fs_info, wtag);
> +		/* self must not be dereferenced past this point. */
> +		trace_btrfs_all_work_done(wq->fs_info, self);
>  	}
>  }
>  
> @@ -304,7 +301,6 @@ static void btrfs_work_helper(struct work_struct *normal_work)
>  	struct btrfs_work *work = container_of(normal_work, struct btrfs_work,
>  					       normal_work);
>  	struct __btrfs_workqueue *wq;
> -	void *wtag;
>  	int need_order = 0;
>  
>  	/*
> @@ -318,8 +314,6 @@ static void btrfs_work_helper(struct work_struct *normal_work)
>  	if (work->ordered_func)
>  		need_order = 1;
>  	wq = work->wq;
> -	/* Safe for tracepoints in case work gets freed by the callback */
> -	wtag = work;
>  
>  	trace_btrfs_work_sched(work);
>  	thresh_exec_hook(wq);
> @@ -327,9 +321,10 @@ static void btrfs_work_helper(struct work_struct *normal_work)
>  	if (need_order) {
>  		set_bit(WORK_DONE_BIT, &work->flags);
>  		run_ordered_work(work);
> +	} else {
> +		/* work must not be dereferenced past this point. */
> +		trace_btrfs_all_work_done(wq->fs_info, work);
>  	}
> -	if (!need_order)
> -		trace_btrfs_all_work_done(wq->fs_info, wtag);
>  }
>  
>  void btrfs_init_work(struct btrfs_work *work, btrfs_func_t func,
> diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
> index 5cb95646b94e..3d61e80d3c6e 100644
> --- a/include/trace/events/btrfs.h
> +++ b/include/trace/events/btrfs.h
> @@ -1388,9 +1388,9 @@ DECLARE_EVENT_CLASS(btrfs__work,
>  );
>  
>  /*
> - * For situiations when the work is freed, we pass fs_info and a tag that that
> - * matches address of the work structure so it can be paired with the
> - * scheduling event.
> + * For situations when the work is freed, we pass fs_info and a tag that that
> + * matches address of the work structure so it can be paired with the scheduling
> + * event. DO NOT add anything here that dereferences wtag.
>   */
>  DECLARE_EVENT_CLASS(btrfs__work__done,
>  
>