Patchwork net: caif: Don't act on notification for non-caif devices

login
register
mail settings
Submitter Sjur Brændeland
Date Jan. 24, 2012, 3:06 p.m.
Message ID <CAJK669ZWJsxnSiyKWFFFUWOA3TaGMY-AmgWuVsYfR=tKQaODgg@mail.gmail.com>
Download mbox | patch
Permalink /patch/137575/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Sjur Brændeland - Jan. 24, 2012, 3:06 p.m.
Hi Sasha,

>> Nack, we have to handle other device types than just ARPHDR_CAIF after
>> introducing CAIF over USB/NCM.
> What my patch did was simply move the type check to above the net_generic call,
> it didn't add any new checks - which according to what you said, you'll need to do anyway.

As I said I, don't think your patch would work. Try to see what happens if
dev->type != ARPHDR_CAIF and caifd != NULL. Then the statement:

	if (caifd == NULL && dev->type != ARPHRD_CAIF)
		return 0;
is very different from:

       if (dev->type != ARPHRD_CAIF)
               return 0;
...
       if (caifd == NULL)
               return 0;

Anyway, another option could be to explicitly check if name space is
initialized,
similar to what net_generic() does,e.g. something like:

--

Regards,
Sjur
--
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
Sasha Levin - Jan. 24, 2012, 3:23 p.m.
On Tue, Jan 24, 2012 at 10:06 AM, Sjur Brændeland <sjurbren@gmail.com> wrote:
> Hi Sasha,
>
>>> Nack, we have to handle other device types than just ARPHDR_CAIF after
>>> introducing CAIF over USB/NCM.
>> What my patch did was simply move the type check to above the net_generic call,
>> it didn't add any new checks - which according to what you said, you'll need to do anyway.
>
> As I said I, don't think your patch would work. Try to see what happens if
> dev->type != ARPHDR_CAIF and caifd != NULL. Then the statement:
>
>        if (caifd == NULL && dev->type != ARPHRD_CAIF)
>                return 0;
> is very different from:
>
>       if (dev->type != ARPHRD_CAIF)
>               return 0;
> ...
>       if (caifd == NULL)
>               return 0;

Right.

> Anyway, another option could be to explicitly check if name space is
> initialized,
> similar to what net_generic() does,e.g. something like:
>
> diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c
> index 673728a..3197bc2 100644
> --- a/net/caif/caif_dev.c
> +++ b/net/caif/caif_dev.c
> @@ -371,6 +371,13 @@ static int caif_device_notify(struct notifier_block *me, un
>        struct cflayer *layer, *link_support;
>        int head_room = 0;
>        struct caif_device_entry_list *caifdevs;
> +       int len;
> +
> +       rcu_read_lock();
> +       len = rcu_dereference(dev_net(dev)->gen)->len;
> +       rcu_read_unlock();
> +       if (caif_net_id > len)
> +               return 0;
>
>        cfg = get_cfcnfg(dev_net(dev));
>        caifdevs = caif_device_list(dev_net(dev));

We could, in that case we'd just need to handle the case where it was
initialized by a device with higher id than CAIF (which we already do
I think), and do it without touching net_generic structure directly.

btw, Why do we store the devices per-namespace instead of globally? Is
it such a big benefit in performance?
--
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
Sasha Levin - Jan. 25, 2012, 4:13 p.m.
On Tue, Jan 24, 2012 at 5:27 PM, Sjur Brændeland
<sjur.brandeland@stericsson.com> wrote:
> net_generic() calls BUG_ON() if called with uninitialized
> network name-space. Add check if net is initialized before
> calling net_generic(). This fixes the following oops:
>
> [  200.752016] kernel BUG at include/net/netns/generic.h:40!
> ...
> [  200.752016]  [<ffffffff825c3cea>] ? get_cfcnfg+0x3a/0x180
> [  200.752016]  [<ffffffff821cf0b0>] ? lockdep_rtnl_is_held+0x10/0x20
> [  200.752016]  [<ffffffff825c41be>] caif_device_notify+0x2e/0x530
> [  200.752016]  [<ffffffff810d61b7>] notifier_call_chain+0x67/0x110
> [  200.752016]  [<ffffffff810d67c1>] raw_notifier_call_chain+0x11/0x20
> [  200.752016]  [<ffffffff821bae82>] call_netdevice_notifiers+0x32/0x60
> [  200.752016]  [<ffffffff821c2b26>] register_netdevice+0x196/0x300
> [  200.752016]  [<ffffffff821c2ca9>] register_netdev+0x19/0x30
> [  200.752016]  [<ffffffff81c1c67a>] loopback_net_init+0x4a/0xa0
> [  200.752016]  [<ffffffff821b5e62>] ops_init+0x42/0x180
> [  200.752016]  [<ffffffff821b600b>] setup_net+0x6b/0x100
> [  200.752016]  [<ffffffff821b6466>] copy_net_ns+0x86/0x110
> [  200.752016]  [<ffffffff810d5789>] create_new_namespaces+0xd9/0x190
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> ---
>
> Hi Sasha,
>
> Do you have any chance to review and test this patch?
> I'd like to get the net namespace handling this right this time ...
>
> Thanks,
> Sjur

Works for me.

Tested-by: Sasha Levin <levinsasha928@gmail.com>
--
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

Patch

diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c
index 673728a..3197bc2 100644
--- a/net/caif/caif_dev.c
+++ b/net/caif/caif_dev.c
@@ -371,6 +371,13 @@  static int caif_device_notify(struct notifier_block *me, un
        struct cflayer *layer, *link_support;
        int head_room = 0;
        struct caif_device_entry_list *caifdevs;
+       int len;
+
+       rcu_read_lock();
+       len = rcu_dereference(dev_net(dev)->gen)->len;
+       rcu_read_unlock();
+       if (caif_net_id > len)
+               return 0;

        cfg = get_cfcnfg(dev_net(dev));
        caifdevs = caif_device_list(dev_net(dev));