Re: [linux-next:master 13298/13495] drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c:183 send_tlb_invalidation() warn: variable dereferenced before check 'fence' (see line 178)

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Jul 23, 2024 at 01:52:46PM -0500, Dan Carpenter wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> head:   dee7f101b64219f512bb2f842227bd04c14efe30
> commit: 61ac035361ae555ee5a17a7667fe96afdde3d59a [13298/13495] drm/xe: Drop xe_gt_tlb_invalidation_wait
> config: i386-randconfig-141-20240722 (https://download.01.org/0day-ci/archive/20240723/202407231049.esig0Fkb-lkp@intel.com/config)
> compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202407231049.esig0Fkb-lkp@intel.com/
> 
> New smatch warnings:
> drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c:183 send_tlb_invalidation() warn: variable dereferenced before check 'fence' (see line 178)
> 
> vim +/fence +183 drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
> 
> fc108a8b759f52 Matthew Brost        2023-01-17  159  static int send_tlb_invalidation(struct xe_guc *guc,
> 332dd0116c82a7 Matthew Brost        2023-01-24  160  				 struct xe_gt_tlb_invalidation_fence *fence,
> 332dd0116c82a7 Matthew Brost        2023-01-24  161  				 u32 *action, int len)
> a9351846d94568 Matthew Brost        2023-01-17  162  {
> a9351846d94568 Matthew Brost        2023-01-17  163  	struct xe_gt *gt = guc_to_gt(guc);
> 501c4255c40935 Radhakrishna Sripada 2024-06-07  164  	struct xe_device *xe = gt_to_xe(gt);
> a9351846d94568 Matthew Brost        2023-01-17  165  	int seqno;
> a9351846d94568 Matthew Brost        2023-01-17  166  	int ret;
> a9351846d94568 Matthew Brost        2023-01-17  167  
> 61ac035361ae55 Matthew Brost        2024-07-19  168  	xe_gt_assert(gt, fence);

We assert the fence is not NULL here. It is invalid call with a NULL fence.

> 61ac035361ae55 Matthew Brost        2024-07-19  169  
> a9351846d94568 Matthew Brost        2023-01-17  170  	/*
> a9351846d94568 Matthew Brost        2023-01-17  171  	 * XXX: The seqno algorithm relies on TLB invalidation being processed
> a9351846d94568 Matthew Brost        2023-01-17  172  	 * in order which they currently are, if that changes the algorithm will
> a9351846d94568 Matthew Brost        2023-01-17  173  	 * need to be updated.
> a9351846d94568 Matthew Brost        2023-01-17  174  	 */
> 565ce72e1c2d54 Matthew Auld         2023-05-24  175  
> a9351846d94568 Matthew Brost        2023-01-17  176  	mutex_lock(&guc->ct.lock);
> 62ad062150c2ab Matthew Brost        2023-01-17  177  	seqno = gt->tlb_invalidation.seqno;
> fc108a8b759f52 Matthew Brost        2023-01-17 @178  	fence->seqno = seqno;
>                                                         ^^^^^^^^^^^^^^^^^^^^
> Dereference
> 
> 501c4255c40935 Radhakrishna Sripada 2024-06-07  179  	trace_xe_gt_tlb_invalidation_fence_send(xe, fence);
> a9351846d94568 Matthew Brost        2023-01-17  180  	action[1] = seqno;
> 332dd0116c82a7 Matthew Brost        2023-01-24  181  	ret = xe_guc_ct_send_locked(&guc->ct, action, len,
> a9351846d94568 Matthew Brost        2023-01-17  182  				    G2H_LEN_DW_TLB_INVALIDATE, 1);
> 38224c00d9c284 Matthew Brost        2023-01-24 @183  	if (!ret && fence) {
>                                                                     ^^^^^
> Checked too late
> 

This check is not needed. Let me post a patch to remove this.

Thanks,
Matt

> 35c8a964398e1c Matthew Auld         2023-07-10  184  		spin_lock_irq(&gt->tlb_invalidation.pending_lock);
> 35c8a964398e1c Matthew Auld         2023-07-10  185  		/*
> 35c8a964398e1c Matthew Auld         2023-07-10  186  		 * We haven't actually published the TLB fence as per
> 35c8a964398e1c Matthew Auld         2023-07-10  187  		 * pending_fences, but in theory our seqno could have already
> 35c8a964398e1c Matthew Auld         2023-07-10  188  		 * been written as we acquired the pending_lock. In such a case
> 35c8a964398e1c Matthew Auld         2023-07-10  189  		 * we can just go ahead and signal the fence here.
> 35c8a964398e1c Matthew Auld         2023-07-10  190  		 */
> 35c8a964398e1c Matthew Auld         2023-07-10  191  		if (tlb_invalidation_seqno_past(gt, seqno)) {
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>