diff mbox

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

Message ID CAJK669acbPLTxTDrk_uqdB2FmewcSA+HF0qEFADgT8YdfNMcpQ@mail.gmail.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Sjur Brændeland Jan. 24, 2012, 10:52 a.m. UTC 
Hi Sasha,

> Since the list of CAIF devices is stored in the net generic struct in each
> net namespace, which is not initialized at that point, we see the following
> BUG():
>
> [  200.752016] kernel BUG at include/net/netns/generic.h:40!
...
> [  200.752016] Call Trace:
> [  200.752016]  [<ffffffff825c3cea>] ? get_cfcnfg+0x3a/0x180
> [  200.752016]  [<ffffffff821cf0b0>] ? lockdep_rtnl_is_held+0x10/0x20
> [  200.752016]  [<ffffffff825c41be>] caif_device_notify+0x2e/0x530

Argh, my bad. This issue has been identified and fixed by David
Woodhouse earlier,
but was reintroduced again by me when adding support for CAIF over NCM.
The CAIF code is handling if net_generic() returns NULL, but I missed that
net_generic() does BUG_ON().

> Instead, we'll first check if the device in the notification is a CAIF device:
>  - If it is - the net generic struct in that namespace must have been already
> initialized.
>  - If not - just ignore it as we don't care about other devices.
>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>

Nack, we have to handle other device types than just ARPHDR_CAIF after
introducing
CAIF over USB/NCM. I'd rather fix this in netns by removing the BUG_ON
and return
NULL. How about this instead:


I'll post a patch for this soon.

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

Comments

Sasha Levin Jan. 24, 2012, 2:49 p.m. UTC | #1 
On Tue, 2012-01-24 at 11:52 +0100, Sjur Brændeland wrote:
> 
> Nack, we have to handle other device types than just ARPHDR_CAIF after
> introducing
> CAIF over USB/NCM. I'd rather fix this in netns by removing the BUG_ON
> and return
> NULL. How about this instead: 
[snip]

I think that doing it this way is wrong for two reasons:

1. The code in net/ assumes net_generic is a trivial dereference and doesn't check that it's not NULL. This means that if anything goes wrong there you'll have a more dangerous NULL deref instead of a BUG().

2. You'll need to add other device to that if() statement anyway, as it currently looks like this:

	cfg = get_cfcnfg(dev_net(dev));
	caifdevs = caif_device_list(dev_net(dev));
	if (!cfg || !caifdevs)
		return 0;

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

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.
diff mbox

Patch

diff --git a/include/net/netns/generic.h b/include/net/netns/generic.h
index 3419bf5..0fc2eea 100644
--- a/include/net/netns/generic.h
+++ b/include/net/netns/generic.h
@@ -37,8 +37,10 @@  static inline void *net_generic(const struct net *net, int id

        rcu_read_lock();
        ng = rcu_dereference(net->gen);
-       BUG_ON(id == 0 || id > ng->len);
-       ptr = ng->ptr[id - 1];
+       if (id == 0 || id > ng->len)
+               ptr = NULL;
+       else
+               ptr = ng->ptr[id - 1];
        rcu_read_unlock();

        return ptr;