netdev - Re: [PATCH v2 12/33] staging: wfx: simplify API coherency check

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 13 Sep 2021 13:02:05 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Jerome Pouiller <Jerome.Pouiller@...abs.com>
Cc:     devel@...verdev.osuosl.org, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "David S . Miller" <davem@...emloft.net>,
        Kalle Valo <kvalo@...eaurora.org>
Subject: Re: [PATCH v2 12/33] staging: wfx: simplify API coherency check

On Mon, Sep 13, 2021 at 10:30:24AM +0200, Jerome Pouiller wrote:
> From: Jérôme Pouiller <jerome.pouiller@...abs.com>
> 
> The 'channel' argument of hif_join() should never be NULL. hif_join()
> does not have the responsibility to recover bug of caller. A call to
> WARN() at the beginning of the function reminds this constraint to the
> developer.
> 
> In current code, if the argument channel is NULL, memory leaks. The new
> code just emit a warning and does not give the illusion that it is
> supported (and indeed a Oops will probably raise a few lines below).
> 
> Signed-off-by: Jérôme Pouiller <jerome.pouiller@...abs.com>
> ---
>  drivers/staging/wfx/hif_tx.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
> index 14b7e047916e..6ffbae32028b 100644
> --- a/drivers/staging/wfx/hif_tx.c
> +++ b/drivers/staging/wfx/hif_tx.c
> @@ -299,10 +299,9 @@ int hif_join(struct wfx_vif *wvif, const struct ieee80211_bss_conf *conf,
>  
>  	WARN_ON(!conf->beacon_int);
>  	WARN_ON(!conf->basic_rates);
> +	WARN_ON(!channel);

This fine.  I'm not trying to make people redo their patches especially
when you're doing a great job as a maintainer.

But generally these WARN_ON()s are pointless.  It's never going to
happen and if we try to handle all the thing which will not happen that's
an impossible task.  But specificically with NULL dereferences, the
WARN() will generate a stack trace and also the Oops will generate a
stack trace.  It's duplicative.

regards,
dan carpenter