linux-kernel - Re: [PATCH v2] staging: r8188eu: Remove _enter/_exit_critical

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <YR5wiOgWXXFqVDH+@kroah.com>
Date:   Thu, 19 Aug 2021 16:54:00 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     Larry Finger <Larry.Finger@...inger.net>,
        Phillip Potter <phil@...lpotter.co.uk>,
        Martin Kaiser <martin@...ser.cx>,
        Michael Straube <straube.linux@...il.com>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] staging: r8188eu: Remove _enter/_exit_critical_mutex()

On Thu, Aug 19, 2021 at 02:49:55PM +0200, Fabio M. De Francesco wrote:
> Remove _enter_critical_mutex() and _exit_critical_mutex(). They are
> unnecessary wrappers, respectively to mutex_lock_interruptible() and
> to mutex_unlock(). They also have an odd interface that takes an
> unused argument named pirqL of type unsigned long.
> Replace them with the in-kernel API. Ignore return values as it was
> in the old code.
> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> ---
> 
> v2: Ignore return values from Mutexes API.
> 
>  drivers/staging/r8188eu/core/rtw_mlme_ext.c     |  5 +++--
>  drivers/staging/r8188eu/hal/usb_ops_linux.c     |  5 +++--
>  drivers/staging/r8188eu/include/osdep_service.h | 13 -------------
>  drivers/staging/r8188eu/os_dep/os_intfs.c       |  5 +++--
>  4 files changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_mlme_ext.c b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> index 5325fe41fbee..9f53cab33333 100644
> --- a/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> +++ b/drivers/staging/r8188eu/core/rtw_mlme_ext.c
> @@ -4359,7 +4359,8 @@ s32 dump_mgntframe_and_wait_ack(struct adapter *padapter, struct xmit_frame *pmg
>  	if (padapter->bSurpriseRemoved || padapter->bDriverStopped)
>  		return -1;
>  
> -	_enter_critical_mutex(&pxmitpriv->ack_tx_mutex, NULL);
> +	if (mutex_lock_interruptible(&pxmitpriv->ack_tx_mutex))
> +		;	/*ignore return value */

Ick, no.  (not to mention the wrong comment style...)

If this really is "criticial", why can it be interrupted?

The existing code is such that the code can be interrupted, but if it
fails, the lock is not gotten, and the CODE CONTINUES AS IF IT IS OK!

So either this is never interruptable (my guess, one almost never needs
interruptable locks in a driver) and should just do a normal mutex lock,
or the code is totally broken and the locking should be revisited
entirely.

But a "blind" change like this is not good, let's get it right...

thanks,

greg k-h