linux-kernel - Re: [PATCH v5 5/5] usb: gadget: udc-core: independent registration of gadgets and gadget drivers

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: <20150706173829.GA20779@saruman.tx.rr.com>
Date:	Mon, 6 Jul 2015 12:38:29 -0500
From:	Felipe Balbi <balbi@...com>
To:	Alan Stern <stern@...land.harvard.edu>
CC:	Ruslan Bilovol <ruslan.bilovol@...il.com>,
	"Balbi, Felipe" <balbi@...com>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Krzysztof Opasiak <k.opasiak@...sung.com>,
	Peter Chen <peter.chen@...escale.com>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	Andrzej Pietrasiewicz <andrzej.p@...sung.com>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>
Subject: Re: [PATCH v5 5/5] usb: gadget: udc-core: independent registration
 of gadgets and gadget drivers

On Sat, Jun 27, 2015 at 07:47:28PM -0400, Alan Stern wrote:
> On Sun, 28 Jun 2015, Ruslan Bilovol wrote:
> 
> > > Weren't you going to replace this loop with a simple list_del()?  IIRC,
> > > this is the third time I have asked you to make this change.
> > 
> > I understand the improvement that replacing this loop with a list_del()
> > may bring for us, but I disagree with doing it in this particular case.
> > 
> > The reason is simple. The usb_gadget_unregister_driver() funciton is
> > externally visible so we can get junk as input. Current implementation
> > checks passed pointer and only after that does list_del(), or
> > returns -EINVAL. Your variant will do list_del() unconditionally, that
> > may cause a kernel crash or unexpected behavior in case of junk
> > passed with *driver. The list_del_init() usage can't help here since
> > there is no way to check that list_head structure is initialized with correct
> > data or contains junk.
> 
> That's right.
> 
> > There is no noticeable performance loss with current implementation,just
> > because current use case is pretty simple: one gadget driver per one UDC,
> > and usually there is only one UDC per machine (or rare cases with few
> > UDCs), thus number of pending gadget drivers is relatively small.
> > We can return back to this discussion if someone needs to register
> > many gadget drivers, and want to improve performance, because
> > there are few existing places (not created by me) in this file that uses
> > same approach of walking through list of registered gadget drivers.
> > 
> > As a bottom line, choosing between stability and little performance
> > improvement, I prefer stability.
> 
> It's not really a question of code size or performance.  As you say,
> the difference in each is minimal.
> 
> It _is_ a question of style.  Adding unnecessary code to check for
> something that shouldn't need to be checked looks bad.  Other kernel
> developers reading that code will notice it and wonder why it's there.  
> That's the argument for getting rid of the loop.

yeah, if someone gives us junk as input, they deserve to oops.
list_del() is enough.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)