linux-kernel - Re: [PATCH] staging: vme_user: vme_bridge.h: Document mutex in vme_dma

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1e74a5ef-7d15-451e-8cb8-2743ef95089a@suswa.mountain>
Date: Fri, 2 Aug 2024 23:23:57 -0500
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Riyan Dhiman <riyandhiman14@...il.com>
Cc: gregkh@...uxfoundation.org, dri-devel@...ts.freedesktop.org,
	linux-fbdev@...r.kernel.org, linux-staging@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: vme_user: vme_bridge.h: Document mutex in
 vme_dma_resource structure

On Sat, Aug 03, 2024 at 05:48:14AM +0530, Riyan Dhiman wrote:
> Adhere to Linux kernel coding style
> 
> Reported by checkpatch:
> 
> CHECK: mutex definition without comment
> 
> Proof for comment:
> 
> 1. The mutex is used to protect access to the 'running' list
> (line 1798 tsi148_dma_list_exec function)
> 	mutex_lock(&ctrlrl->mtx);
> 	if (!list_empty(&ctrlr->running)) {
> 		mutex_unlock(&ctrlr->mtx);
> 		return -EBUSY;
> 	}

Why did you chop out the "channel = ctrlr->number;" line?  That code
looks like this:

drivers/staging/vme_user/vme_tsi148.c
  1798          mutex_lock(&ctrlr->mtx);
  1799  
  1800          channel = ctrlr->number;
  1801  
  1802          if (!list_empty(&ctrlr->running)) {
  1803                  /*
  1804                   * XXX We have an active DMA transfer and currently haven't
  1805                   *     sorted out the mechanism for "pending" DMA transfers.
  1806                   *     Return busy.
  1807                   */
  1808                  /* Need to add to pending here */
  1809                  mutex_unlock(&ctrlr->mtx);
  1810                  return -EBUSY;
  1811          }
  1812  
  1813          list_add(&list->list, &ctrlr->running);


The first line after we take a lock and the last line before we drop
the lock are hopefully chosen because they need to be protected by the
lock.

>   This prevents race conditions when multiple threads attempt to start DMA
>   operations simultaneously.
> 
> 2. It's also used when removing DMA list from running list:
> (line 1862 tsi148_dma_list_exec function)
> 	mutex_lock(&ctrlr->mtx);
> 	list_del(&list->list);
> 	mutex_unlock(&ctrlr->mtx);
>   Ensuring thread-safe modification of the controller's state.
> 
> Without this mutex, concurrent access to the DMA controller's state could
> lead to data corruption or inconsistant state.
>

It's also used in drivers/staging/vme_user/vme.c
What you should do is rename the mutex from mtx to XXX_mtx and then
rename all the places where it is used.  Keep renaming until the driver
builds.

XXX_mtx is obviously not the correct name.  But "mtx" is vague and
useless.  There are 3 other locks in this header file which have the
same name so not only is it useless as a descriptive name, it's also
useless for searching.

Since you say that it is "protect access to the 'running' list", then
that means you need to check all the places where the running list is
accessed and ensure that the lock is held.  Or if it's not, what makes
that thread safe?

These comments are supposed to help us understand the locking but it
feels like we have a long way to go before it's fully understood.

> Signed-off-by: Riyan Dhiman <riyandhiman14@...il.com>
> ---
>  drivers/staging/vme_user/vme_bridge.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/vme_user/vme_bridge.h b/drivers/staging/vme_user/vme_bridge.h
> index 9bdc41bb6602..bb3750b40eb1 100644
> --- a/drivers/staging/vme_user/vme_bridge.h
> +++ b/drivers/staging/vme_user/vme_bridge.h
> @@ -61,6 +61,7 @@ struct vme_dma_list {
>  struct vme_dma_resource {
>  	struct list_head list;
>  	struct vme_bridge *parent;
> +	/* Mutex to protect DMA controller resources and ensure thread-safe operations */

"resources" is too vague.  "ensure thread-safe operations" is obvious
and doesn't need to be said.

regards,
dan carpenter