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!
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 <redacted>
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:
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.
--
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:
On Tue, Jan 24, 2012 at 10:06 AM, Sjur Brændeland [off-list ref] wrote:
Hi Sasha,
quoted
quoted
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.
quoted hunk
Anyway, another option could be to explicitly check if name space is
initialized,
similar to what net_generic() does,e.g. something like:
@@ -371,6 +371,13 @@ static int caif_device_notify(struct notifier_block *me, unstructcflayer*layer,*link_support;inthead_room=0;structcaif_device_entry_list*caifdevs;+intlen;++rcu_read_lock();+len=rcu_dereference(dev_net(dev)->gen)->len;+rcu_read_unlock();+if(caif_net_id>len)+return0;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?