re: netdevice wanrouter: Convert directly reference of netdev->priv

re: netdevice wanrouter: Convert directly reference of netdev->priv

[Dan Carpenter]

Hello Wang Chen,

The patch 7be6065b39c3: "netdevice wanrouter: Convert directly
reference of netdev->priv" from Nov 20, 2008, leads to the following
Smatch warning:
net/wanrouter/wanmain.c:610 wanrouter_device_new_if()
	 error: potential NULL dereference 'dev'.

This is an old patch from 2008.  It removed the allocation in
wanrouter_device_new_if() so it looks like wanrouter has been completely
broken for four years.

@@ -589,10 +591,6 @@ static int wanrouter_device_new_if(struct wan_device *wandev,
                err = -EPROTONOSUPPORT;
                goto out;
        } else {
-               dev = kzalloc(sizeof(struct net_device), GFP_KERNEL);
-               err = -ENOBUFS;
-               if (dev == NULL)
-                       goto out;
                err = wandev->new_if(wandev, dev, cnf);

"dev" is still NULL after the call to ->new_if().

        }

Here is what the code looks like now:

net/wanrouter/wanmain.c
   590          if (cnf->config_id == WANCONFIG_MPPP) {
   591                  printk(KERN_INFO "%s: Wanpipe Mulit-Port PPP support has not been compiled in!\n",
   592                                  wandev->name);
   593                  err = -EPROTONOSUPPORT;
   594                  goto out;
   595          } else {

We were supposed to allocate "dev" here.

   596                  err = wandev->new_if(wandev, dev, cnf);
   597          }
   598  
   599          if (!err) {
   600                  /* Register network interface. This will invoke init()
   601                   * function supplied by the driver.  If device registered
   602                   * successfully, add it to the interface list.
   603                   */
   604  
   605  #ifdef WANDEBUG
   606                  printk(KERN_INFO "%s: registering interface %s...\n",
   607                         wanrouter_modname, dev->name);
   608  #endif
   609  
   610                  err = register_netdev(dev);
                              ^^^^^^^^^^^^^^^^^^^^

The kernel will always oops inside the call to register_netdev() because
"dev" is still NULL.

I suspect we should just revert the patch?

regards,
dan carpenter

Re: netdevice wanrouter: Convert directly reference of netdev->priv

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Mon, 3 Dec 2012 12:04:05 +0300

> I suspect we should just revert the patch?

That is absolutely something we cannot do.

netdev->priv no longer exists, first of all.

And the whole point of this change was to allow us to
remove any and all references to netdev->priv, exactly
so that we could remove it altogether.

If you look at the commit in question, the idea is to
allocate the netdev in the ->new_if() callback.

And that's in fact what happens.

What's missing at the top level is something like:

	dev = wandev->dev;

And perhaps removing the 'dev' argument to the ->new_if()
method since it obviously is always NULL and never actually
used.

Thanks.

Re: netdevice wanrouter: Convert directly reference of netdev->priv

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Tue, 11 Dec 2012 13:43:10 -0500 (EST)

> What's missing at the top level is something like:
> 
> 	dev = wandev->dev;
> 
> And perhaps removing the 'dev' argument to the ->new_if()
> method since it obviously is always NULL and never actually
> used.

And for the record, wangchen's email bounces so it's very
unlikely we'll see any response from him.

Re: netdevice wanrouter: Convert directly reference of netdev->priv

[Paul Gortmaker]

On Mon, Dec 3, 2012 at 4:04 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Hello Wang Chen,
>
> The patch 7be6065b39c3: "netdevice wanrouter: Convert directly
> reference of netdev->priv" from Nov 20, 2008, leads to the following
> Smatch warning:
> net/wanrouter/wanmain.c:610 wanrouter_device_new_if()
>          error: potential NULL dereference 'dev'.
>
> This is an old patch from 2008.  It removed the allocation in
> wanrouter_device_new_if() so it looks like wanrouter has been completely
> broken for four years.

Hi Dan,

Crap -- wishing I'd seen this earlier.  There was an RFC patch for
sending wanrouter to the bitbucket from Joe Perches, but aside
from the obvious build failures in it that Dave found (and I fixed)
there wasn't any real feedback (either positive or negative) to it:

http://patchwork.ozlabs.org/patch/198830/

Knowing it has been non-functional for ~4 years is I think a key
bit of information in justifying a removal, so folks like yourself
and JuliaL don't waste cycles fixing/auditing dead code.  But it
will need to be 3.9 material now, it seems.

Paul.
--

>
> @@ -589,10 +591,6 @@ static int wanrouter_device_new_if(struct wan_device *wandev,
>                 err = -EPROTONOSUPPORT;
>                 goto out;
>         } else {
> -               dev = kzalloc(sizeof(struct net_device), GFP_KERNEL);
> -               err = -ENOBUFS;
> -               if (dev == NULL)
> -                       goto out;
>                 err = wandev->new_if(wandev, dev, cnf);
>
> "dev" is still NULL after the call to ->new_if().
>
>         }
>
> Here is what the code looks like now:
>
> net/wanrouter/wanmain.c
>    590          if (cnf->config_id == WANCONFIG_MPPP) {
>    591                  printk(KERN_INFO "%s: Wanpipe Mulit-Port PPP support has not been compiled in!\n",
>    592                                  wandev->name);
>    593                  err = -EPROTONOSUPPORT;
>    594                  goto out;
>    595          } else {
>
> We were supposed to allocate "dev" here.
>
>    596                  err = wandev->new_if(wandev, dev, cnf);
>    597          }
>    598
>    599          if (!err) {
>    600                  /* Register network interface. This will invoke init()
>    601                   * function supplied by the driver.  If device registered
>    602                   * successfully, add it to the interface list.
>    603                   */
>    604
>    605  #ifdef WANDEBUG
>    606                  printk(KERN_INFO "%s: registering interface %s...\n",
>    607                         wanrouter_modname, dev->name);
>    608  #endif
>    609
>    610                  err = register_netdev(dev);
>                               ^^^^^^^^^^^^^^^^^^^^
>
> The kernel will always oops inside the call to register_netdev() because
> "dev" is still NULL.
>
> I suspect we should just revert the patch?
>
> regards,
> dan carpenter
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html