linux-kernel - Re: [PATCH v2] staging: wfx: use container

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]

Date:   Mon, 11 Apr 2022 23:57:39 -0400
From:   Jaehee Park <jhpark1013@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Jérôme Pouiller <jerome.pouiller@...abs.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        outreachy@...ts.linux.dev, Stefano Brivio <sbrivio@...hat.com>
Subject: Re: [PATCH v2] staging: wfx: use container_of() to get vif

On Fri, Apr 08, 2022 at 09:19:36AM +0300, Dan Carpenter wrote:
> On Thu, Apr 07, 2022 at 11:23:49PM -0400, Jaehee Park wrote:
> > diff --git a/drivers/staging/wfx/wfx.h b/drivers/staging/wfx/wfx.h
> > index 6594cc647c2f..78f2a416fe4f 100644
> > --- a/drivers/staging/wfx/wfx.h
> > +++ b/drivers/staging/wfx/wfx.h
> > @@ -25,6 +25,8 @@
> >  #define USEC_PER_TXOP 32 /* see struct ieee80211_tx_queue_params */
> >  #define USEC_PER_TU 1024
> >  
> > +#define wvif_to_vif(ptr)(container_of((void *)ptr, struct ieee80211_vif, drv_priv))
> > +
> 
> Better to make this a function.
> 

Hi Dan, Thank you for your comments. To make sure I'm understanding your
concerns correctly, do you mean I should define a function instead of
using this macros here?

> Stefano's comments are correct.  It would have saved space with the 80
> limit to do a "struct ieee80211_vif *vif = wvif_to_vif();" at the start

Got it. I implemented this on the next patch (v3) that I will be sending
out soon. 

> of the function.  Also dereferencing the results of a function call
> like this, "frob(foo)->bar", without checking makes me itch.  If it's
> at the top of the function then that's kind of different.  I normally
> assume that the functions in the declaration block cannot fail.  From
> analysing static checker warnings, putting functions which can fail in
> that the declaration block is risky.
> 

The frob(foo)->bar in my case would be wvif_to_vif(wvif)->type?

> It's always better to test things but this patch looks correct to me:
> 
> The add interface does:
> 
> 	struct wfx_vif *wvif = (struct wfx_vif *)vif->drv_priv
>         ...
> 	wvif->vif = vif;
> 
> The remove interface does:
> 	wvif->vif = NULL;
> 
> Those are the only places where ->vif is set container_of() will always
> work.

Yes this is true. The add and remove interface was the inspiration point
for this patch.
> 
> regards,
> dan carpenter
>