Re: [PATCH] net: hso: register netdev later to avoid a race condition

[Andreas Kemnade]

On Thu, 27 Apr 2017 10:44:01 +0200
Johan Hovold <johan@xxxxxxxxxx> wrote:

> On Wed, Apr 26, 2017 at 07:26:40PM +0200, Andreas Kemnade wrote:
> > If the netdev is accessed before the urbs are initialized,
> > there will be NULL pointer dereferences. That is avoided by
> > registering it when it is fully initialized.
> 
> > Reported-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
> > Signed-off-by: Andreas Kemnade <andreas@xxxxxxxxxxxx>
> > ---
> >  drivers/net/usb/hso.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> > index 93411a3..00067a0 100644
> > --- a/drivers/net/usb/hso.c
> > +++ b/drivers/net/usb/hso.c
> > @@ -2534,13 +2534,6 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
> >  	SET_NETDEV_DEV(net, &interface->dev);
> >  	SET_NETDEV_DEVTYPE(net, &hso_type);
> >  
> > -	/* registering our net device */
> > -	result = register_netdev(net);
> > -	if (result) {
> > -		dev_err(&interface->dev, "Failed to register device\n");
> > -		goto exit;
> > -	}
> > -
> >  	/* start allocating */
> >  	for (i = 0; i < MUX_BULK_RX_BUF_COUNT; i++) {
> >  		hso_net->mux_bulk_rx_urb_pool[i] = usb_alloc_urb(0, GFP_KERNEL);
> > @@ -2560,6 +2553,13 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
> >  
> >  	add_net_device(hso_dev);
> >  
> > +	/* registering our net device */
> > +	result = register_netdev(net);
> > +	if (result) {
> > +		dev_err(&interface->dev, "Failed to register device\n");
> > +		goto exit;
> 
> This all looks good, but you should consider cleaning up the error
> handling of this function as a follow-up as we should not be
> deregistering netdevs that have never been registered (e.g. if a
> required endpoint is missing or if registration fails for some reason).
> 
> But just to be clear, this problem existed also before this change.
> 
Just to check wether I am understanding this correctly. In your opinion
this patch is good for now. And later when it is applied, there should
be an additional error handling cleanup patch.

Regards,
Andreas

Attachment: pgpRhbfF1jTtD.pgp
Description: OpenPGP digital signature