linux-kernel - Re: [PATCH 1/2] staging: rtl8723bs: Convert LIST_CONTAINOR to use kernel 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]

Message-ID: <1497437782.18751.47.camel@perches.com>
Date:   Wed, 14 Jun 2017 03:56:22 -0700
From:   Joe Perches <joe@...ches.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org, devel@...verdev.osuosl.org,
        Hans de Goede <hdegoede@...hat.com>,
        Larry Finger <Larry.Finger@...inger.net>
Subject: Re: [PATCH 1/2] staging: rtl8723bs: Convert LIST_CONTAINOR to use
 kernel container_of

On Wed, 2017-06-14 at 12:48 +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 14, 2017 at 03:38:21AM -0700, Joe Perches wrote:
> > On Wed, 2017-06-14 at 12:18 +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Jun 13, 2017 at 02:12:56PM -0700, Joe Perches wrote:
> > > > These are similar macros so use the normal kernel one.
> > > > 
> > > > As well, there are odd games being played with casting a plist to
> > > > a union recv_frame by using LIST_CONTAINOR.  Just use a direct cast
> > > > to union recv_frame instead.
> > 
> > []
> > > > diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c b/drivers/staging/rtl8723bs/core/rtw_recv.c
> > 
> > []
> > > > @@ -129,7 +129,7 @@ union recv_frame *_rtw_alloc_recvframe(struct __queue *pfree_recv_queue)
> > > >  
> > > >  		plist = get_next(phead);
> > > >  
> > > > -		precvframe = LIST_CONTAINOR(plist, union recv_frame, u);
> > > > +		precvframe = (union recv_frame *)plist;
> > > 
> > > No, you are "assuming" that the list_head is going to stay the first
> > > object of this structure, and what if it isn't?
> > > 
> > > Just use container_of, that way at least you get the type safeness of
> > > the call, and if something changes in the future, you don't instantly
> > > break the code everywhere without knowing it.
> > 
> > It's a named union u and it's exactly at the beginning.
> > It's unlikely to change.
> > 
> > The container_of macro doesn't work here as there it has
> > a BUILD_BUG_ON_MSG and there are pointer type mismatches
> > on those uses.
> 
> Yeah, but random pointer casts like this are not good, I don't want to
> see that here, sorry.
> 
> Fix up the pointer mismatches, if there are any, as that implies there
> are bugs here...

<shrug>.

No thanks.

That's what the current code is doing now.

Ignore the whole thing if you want.

Someone else might fix it eventually, but more
likely the current rtl8723bs driver will be
dropped and completely rewritten anyway.