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

[Alan Stern]

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.

Your argument for keeping the loop is to prevent crashes when the 
function is called by a buggy driver.  Lots of other people have made 
similar arguments in the past.  But that's not how the kernel is 
written -- we don't go out of our way to cover up potential bugs.

If a driver is buggy, we _want_ it to cause a crash!  How else are we 
going to know about the bug?  Sure, there might be other symptoms that 
someone might eventually notice.  But nobody can miss an oops.

In short, don't try to protect against mistakes in other people's code.  
Let them stand out so they can get fixed!

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/