loopback: Force LOOPBACK_IFINDEX for registration

Submitted by Serhey Popovych on June 16, 2017, 12:10 p.m.

Details

Message ID 1497615003-24762-1-git-send-email-serhe.popovych@gmail.com
State Changes Requested
Delegated to: David Miller
Headers show

Commit Message

Serhey Popovych June 16, 2017, 12:10 p.m.
Now with commit 9c7dafb (net: Allow to create links with
given ifindex) support registration of network devices
with specific ifindex is added.

We can force loopback network device index before call to
register_netdev() to ensure we always configure it with
LOOPBACK_IFINDEX.

Kill BUG_ON() since system can continue without network
namespace failed in loopback init path, unless it is
init_net namespace where we panic() anyway.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 drivers/net/loopback.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Sergei Shtylyov June 17, 2017, 11:21 a.m.
Hello!

On 6/16/2017 3:10 PM, Serhey Popovych wrote:

> Now with commit 9c7dafb (net: Allow to create links with

    The commit citing style is standardized: 12-digit SHA1 and the summary 
enclosed in ("").

> given ifindex) support registration of network devices

    Support for.

> with specific ifindex is added.

    Was added.

> We can force loopback network device index before call to
> register_netdev() to ensure we always configure it with
> LOOPBACK_IFINDEX.
>
> Kill BUG_ON() since system can continue without network
> namespace failed in loopback init path, unless it is
> init_net namespace where we panic() anyway.
>
> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
[...]

MBR, Sergei
David Miller June 19, 2017, 6:41 p.m.
From: Serhey Popovych <serhe.popovych@gmail.com>
Date: Fri, 16 Jun 2017 15:10:03 +0300

> Now with commit 9c7dafb (net: Allow to create links with
> given ifindex) support registration of network devices
> with specific ifindex is added.
> 
> We can force loopback network device index before call to
> register_netdev() to ensure we always configure it with
> LOOPBACK_IFINDEX.
> 
> Kill BUG_ON() since system can continue without network
> namespace failed in loopback init path, unless it is
> init_net namespace where we panic() anyway.
> 
> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>

Is the BUG_ON() triggering, if so why?

It looks to me that unless there is a bug, this assignment
is unnecessary.
Serhey Popovych June 20, 2017, 10:32 a.m.
> From: Serhey Popovych <serhe.popovych@gmail.com>
> Date: Fri, 16 Jun 2017 15:10:03 +0300
> 
>> Now with commit 9c7dafb (net: Allow to create links with
>> given ifindex) support registration of network devices
>> with specific ifindex is added.
>>
>> We can force loopback network device index before call to
>> register_netdev() to ensure we always configure it with
>> LOOPBACK_IFINDEX.
>>
>> Kill BUG_ON() since system can continue without network
>> namespace failed in loopback init path, unless it is
>> init_net namespace where we panic() anyway.
>>
>> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
> 
> Is the BUG_ON() triggering, if so why?
> 
> It looks to me that unless there is a bug, this assignment
> is unnecessary.
> 
Okay, get your position, thanks!
Serhey Popovych June 21, 2017, 9:35 a.m.
> 
>> From: Serhey Popovych <serhe.popovych@gmail.com>
>> Date: Fri, 16 Jun 2017 15:10:03 +0300
>>
>>> Now with commit 9c7dafb (net: Allow to create links with
>>> given ifindex) support registration of network devices
>>> with specific ifindex is added.
>>>
>>> We can force loopback network device index before call to
>>> register_netdev() to ensure we always configure it with
>>> LOOPBACK_IFINDEX.
>>>
>>> Kill BUG_ON() since system can continue without network
>>> namespace failed in loopback init path, unless it is
>>> init_net namespace where we panic() anyway.
>>>
>>> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
>>
>> Is the BUG_ON() triggering, if so why?
>>
>> It looks to me that unless there is a bug, this assignment
>> is unnecessary.
>>
> Okay, get your position, thanks!
> 

Sorry for raising again, but my objectives following:

BUG_ON() might be compiled out when CONFIG_BUG=n.
That's open possibility in buggy setups to have
"lo" with ->ifindex other than LOOPBACK_IFINDEX.

By forcing register_netdevice() via setting
ifindex = LOOPBACK_IFINDEX we address CONFIG_BUG=n
case.

We can leave BUG_ON(), but add ifindex = LOOPBACK_IFINDEX
to ensure register_netdevice() operates properly.

What do you think?
David Miller June 21, 2017, 3:44 p.m.
From: Serhey Popovych <serhe.popovych@gmail.com>
Date: Wed, 21 Jun 2017 12:35:12 +0300

> 
>> 
>>> From: Serhey Popovych <serhe.popovych@gmail.com>
>>> Date: Fri, 16 Jun 2017 15:10:03 +0300
>>>
>>>> Now with commit 9c7dafb (net: Allow to create links with
>>>> given ifindex) support registration of network devices
>>>> with specific ifindex is added.
>>>>
>>>> We can force loopback network device index before call to
>>>> register_netdev() to ensure we always configure it with
>>>> LOOPBACK_IFINDEX.
>>>>
>>>> Kill BUG_ON() since system can continue without network
>>>> namespace failed in loopback init path, unless it is
>>>> init_net namespace where we panic() anyway.
>>>>
>>>> Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
>>>
>>> Is the BUG_ON() triggering, if so why?
>>>
>>> It looks to me that unless there is a bug, this assignment
>>> is unnecessary.
>>>
>> Okay, get your position, thanks!
>> 
> 
> Sorry for raising again, but my objectives following:
> 
> BUG_ON() might be compiled out when CONFIG_BUG=n.
> That's open possibility in buggy setups to have
> "lo" with ->ifindex other than LOOPBACK_IFINDEX.
> 
> By forcing register_netdevice() via setting
> ifindex = LOOPBACK_IFINDEX we address CONFIG_BUG=n
> case.
> 
> We can leave BUG_ON(), but add ifindex = LOOPBACK_IFINDEX
> to ensure register_netdevice() operates properly.
> 
> What do you think?

I don't understand what you are trying to achieve.

BUG_ON() is an assertion.

It checks that an invariant holds.

Whether the check is enabled or not has no bearing on the
invariant.

There is no reason to make the assignment if it is supposed
to be set properly already, period.  The assertion states
that it is supposed to be set properly when we get to this
code.

Patch hide | download patch | download mbox

diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index 3061249..d7233aa 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -210,12 +210,13 @@  static __net_init int loopback_net_init(struct net *net)
 	if (!dev)
 		goto out;
 
+	dev->ifindex = LOOPBACK_IFINDEX;
+
 	dev_net_set(dev, net);
 	err = register_netdev(dev);
 	if (err)
 		goto out_free_netdev;
 
-	BUG_ON(dev->ifindex != LOOPBACK_IFINDEX);
 	net->loopback_dev = dev;
 	return 0;