* [PATCH RFC net-next] net: Allow name change of IFF_UP interfaces
@ 2017-08-09 10:42 Vitaly Kuznetsov
2017-08-09 12:29 ` 吉藤英明
0 siblings, 1 reply; 6+ messages in thread
From: Vitaly Kuznetsov @ 2017-08-09 10:42 UTC (permalink / raw)
To: netdev; +Cc: linux-kernel, David S. Miller, Eric Dumazet, Stephen Hemminger
Recent 'transparenf VF' changes to netvsc driver made VF interfaces
register as netvsc interface slaves upon appearance. This led to udev
not being able to rename the interface according to the 'predictable
interface names' scheme:
kernel: mlx4_core 0002:00:02.0 eth2: joined to eth1
kernel: hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: VF
registering: eth2
kernel: mlx4_en: eth2: Link Up
kernel: hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: Data path
switched to VF: eth2
systemd-udevd[1785]: Error changing net interface name 'eth2' to
'enP2p0s2': Device or resource busy
systemd-udevd[1785]: could not rename interface '5' from 'eth2' to
'enP2p0s2': Device or resource busy
What happens is: __netvsc_vf_setup() does dev_open() for the VF device and
the consecutive dev_change_name() fails with -EBUSY because of the
(dev->flags & IFF_UP) check. The history of this code predates git so I
wasn't able to figure out when and why the check was added, everything
seems to work fine without it. dev_change_name() has only two call sites,
both hold rtnl_lock.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
RFC: I'm probably miossing something obvious and the check can't be just
dropped. Stephen suggested a different solution to the isuue:
https://www.spinics.net/lists/netdev/msg448243.html but it has its own
drawbacks.
---
net/core/dev.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 1d75499add72..c608e233a78a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1186,8 +1186,6 @@ int dev_change_name(struct net_device *dev, const char *newname)
BUG_ON(!dev_net(dev));
net = dev_net(dev);
- if (dev->flags & IFF_UP)
- return -EBUSY;
write_seqcount_begin(&devnet_rename_seq);
--
2.13.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RFC net-next] net: Allow name change of IFF_UP interfaces
2017-08-09 10:42 [PATCH RFC net-next] net: Allow name change of IFF_UP interfaces Vitaly Kuznetsov
@ 2017-08-09 12:29 ` 吉藤英明
2017-08-09 15:05 ` Vitaly Kuznetsov
0 siblings, 1 reply; 6+ messages in thread
From: 吉藤英明 @ 2017-08-09 12:29 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: network dev, Linux Kernel Mailing List, David S. Miller,
Eric Dumazet, Stephen Hemminger
2017-08-09 19:42 GMT+09:00 Vitaly Kuznetsov <vkuznets@redhat.com>:
> What happens is: __netvsc_vf_setup() does dev_open() for the VF device and
> the consecutive dev_change_name() fails with -EBUSY because of the
> (dev->flags & IFF_UP) check. The history of this code predates git so I
> wasn't able to figure out when and why the check was added, everything
> seems to work fine without it. dev_change_name() has only two call sites,
> both hold rtnl_lock.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> RFC: I'm probably miossing something obvious and the check can't be just
> dropped. Stephen suggested a different solution to the isuue:
> https://www.spinics.net/lists/netdev/msg448243.html but it has its own
> drawbacks.
> ---
> net/core/dev.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1d75499add72..c608e233a78a 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1186,8 +1186,6 @@ int dev_change_name(struct net_device *dev, const char *newname)
> BUG_ON(!dev_net(dev));
>
> net = dev_net(dev);
> - if (dev->flags & IFF_UP)
> - return -EBUSY;
>
> write_seqcount_begin(&devnet_rename_seq);
I think people expect the name won't change while up
and I don't think it is a good idea to allow changing the
name while the interface is up.
--yoshfuji
>
> --
> 2.13.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC net-next] net: Allow name change of IFF_UP interfaces
2017-08-09 12:29 ` 吉藤英明
@ 2017-08-09 15:05 ` Vitaly Kuznetsov
2017-08-09 16:10 ` Andrew Lunn
0 siblings, 1 reply; 6+ messages in thread
From: Vitaly Kuznetsov @ 2017-08-09 15:05 UTC (permalink / raw)
To: 吉藤英明
Cc: network dev, Linux Kernel Mailing List, David S. Miller,
Eric Dumazet, Stephen Hemminger
吉藤英明 <hideaki.yoshifuji@miraclelinux.com> writes:
> 2017-08-09 19:42 GMT+09:00 Vitaly Kuznetsov <vkuznets@redhat.com>:
>> What happens is: __netvsc_vf_setup() does dev_open() for the VF device and
>> the consecutive dev_change_name() fails with -EBUSY because of the
>> (dev->flags & IFF_UP) check. The history of this code predates git so I
>> wasn't able to figure out when and why the check was added, everything
>> seems to work fine without it. dev_change_name() has only two call sites,
>> both hold rtnl_lock.
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>> RFC: I'm probably miossing something obvious and the check can't be just
>> dropped. Stephen suggested a different solution to the isuue:
>> https://www.spinics.net/lists/netdev/msg448243.html but it has its own
>> drawbacks.
>> ---
>> net/core/dev.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 1d75499add72..c608e233a78a 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -1186,8 +1186,6 @@ int dev_change_name(struct net_device *dev, const char *newname)
>> BUG_ON(!dev_net(dev));
>>
>> net = dev_net(dev);
>> - if (dev->flags & IFF_UP)
>> - return -EBUSY;
>>
>> write_seqcount_begin(&devnet_rename_seq);
>
> I think people expect the name won't change while up
> and I don't think it is a good idea to allow changing the
> name while the interface is up.
I understand the 'legacy' concern but at the same time we don't want to
have aftificial limitations too. Name change, in particular, doesn't
happen 'under the hood' -- someone privileged enough needs to request
the change.
Can you think of any particular real world scenarios which are broken by
the change?
--
Vitaly
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC net-next] net: Allow name change of IFF_UP interfaces
2017-08-09 15:05 ` Vitaly Kuznetsov
@ 2017-08-09 16:10 ` Andrew Lunn
2017-08-10 8:41 ` Vitaly Kuznetsov
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2017-08-09 16:10 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: ????????????, network dev, Linux Kernel Mailing List,
David S. Miller, Eric Dumazet, Stephen Hemminger
> I understand the 'legacy' concern but at the same time we don't want to
> have aftificial limitations too. Name change, in particular, doesn't
> happen 'under the hood' -- someone privileged enough needs to request
> the change.
>
> Can you think of any particular real world scenarios which are broken by
> the change?
How about:
man 8 dhclient-script
The interface name is passed in $interface to the scripts. Do we get
the old name or the new name? I suspect scripts are going to break if
they are given the old name, which no longer exists.
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC net-next] net: Allow name change of IFF_UP interfaces
2017-08-09 16:10 ` Andrew Lunn
@ 2017-08-10 8:41 ` Vitaly Kuznetsov
2017-08-10 14:10 ` Andrew Lunn
0 siblings, 1 reply; 6+ messages in thread
From: Vitaly Kuznetsov @ 2017-08-10 8:41 UTC (permalink / raw)
To: Andrew Lunn
Cc: ????????????, network dev, Linux Kernel Mailing List,
David S. Miller, Eric Dumazet, Stephen Hemminger
Andrew Lunn <andrew@lunn.ch> writes:
>> I understand the 'legacy' concern but at the same time we don't want to
>> have aftificial limitations too. Name change, in particular, doesn't
>> happen 'under the hood' -- someone privileged enough needs to request
>> the change.
>>
>> Can you think of any particular real world scenarios which are broken by
>> the change?
>
> How about:
>
> man 8 dhclient-script
>
> The interface name is passed in $interface to the scripts. Do we get
> the old name or the new name? I suspect scripts are going to break if
> they are given the old name, which no longer exists.
Yes but why would anyone change interface name while dhclient-script is
running? Things will also go wrong if you try bringing interface down
during the run or do some other configuration, right? Running multiple
configuration tools at the same moment is a bad idea, you never know
what you're gonna end up with.
As I see it, checks in kernel we have are meant to protect kernel
itself, not to disallow all user<->kernel interactions leading to
imperfect result.
(AFAIU) If we remove the check nothing is going to change: udev will
still be renaming interfaces before bringing them up. In netvsc case
users are not supposed to configure the VF interface at all, it just
becomes a slave of netvsc interface.
--
Vitaly
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RFC net-next] net: Allow name change of IFF_UP interfaces
2017-08-10 8:41 ` Vitaly Kuznetsov
@ 2017-08-10 14:10 ` Andrew Lunn
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Lunn @ 2017-08-10 14:10 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: ????????????, network dev, Linux Kernel Mailing List,
David S. Miller, Eric Dumazet, Stephen Hemminger
> >> Can you think of any particular real world scenarios which are broken by
> >> the change?
> >
> > How about:
> >
> > man 8 dhclient-script
> >
> > The interface name is passed in $interface to the scripts. Do we get
> > the old name or the new name? I suspect scripts are going to break if
> > they are given the old name, which no longer exists.
>
> Yes but why would anyone change interface name while dhclient-script is
> running? Things will also go wrong if you try bringing interface down
> during the run or do some other configuration, right?
dhclient already handles the interface going down. sendto/recvfrom
fails and returns an error code. As far as i remember, dhclient then
exits.
> Running multiple configuration tools at the same moment is a bad
> idea, you never know what you're gonna end up with.
It could be argued that configuring an interface vs renaming an
interface are at different levels.
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-08-10 14:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-09 10:42 [PATCH RFC net-next] net: Allow name change of IFF_UP interfaces Vitaly Kuznetsov
2017-08-09 12:29 ` 吉藤英明
2017-08-09 15:05 ` Vitaly Kuznetsov
2017-08-09 16:10 ` Andrew Lunn
2017-08-10 8:41 ` Vitaly Kuznetsov
2017-08-10 14:10 ` Andrew Lunn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox