| Message ID | 20170809104202.30959-1-vkuznets@redhat.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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 >
吉藤英明 <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?
> 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
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.
On Thu, 2017-08-10 at 10:41 +0200, Vitaly Kuznetsov wrote: > 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. Are we sending an event if device name is changed ? If yes, your patch is fine. If not, daemons would not be aware the need to refresh their view of the world.
Eric Dumazet <eric.dumazet@gmail.com> writes: > On Thu, 2017-08-10 at 10:41 +0200, Vitaly Kuznetsov wrote: >> 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. > > Are we sending an event if device name is changed ? > We are - rtnetlink_event() does the job. We, however, don't have a special IFLA_EVENT_* for name change and end up with IFLA_EVENT_NONE. > If yes, your patch is fine. > > If not, daemons would not be aware the need to refresh their view of the > world. Yes but AFAIU daemons may need to do the same refresh when the interface is down too (and, hopefully, they do it already).
> >> 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
> We are - rtnetlink_event() does the job. We, however, don't have a > special IFLA_EVENT_* for name change and end up with IFLA_EVENT_NONE. What is in this event? Old and new name? Just the new name? Andrew
Andrew Lunn <andrew@lunn.ch> writes: >> We are - rtnetlink_event() does the job. We, however, don't have a >> special IFLA_EVENT_* for name change and end up with IFLA_EVENT_NONE. > > What is in this event? Old and new name? Just the new name? Basically, it's everything we know about the interface - type, index, name, mtu, qdisc, ... - see rtnl_fill_ifinfo(). Back to your question - it's only the new name.
On Thu, Aug 10, 2017 at 05:24:55PM +0200, Vitaly Kuznetsov wrote: > Andrew Lunn <andrew@lunn.ch> writes: > > >> We are - rtnetlink_event() does the job. We, however, don't have a > >> special IFLA_EVENT_* for name change and end up with IFLA_EVENT_NONE. > > > > What is in this event? Old and new name? Just the new name? > > Basically, it's everything we know about the interface - type, index, > name, mtu, qdisc, ... - see rtnl_fill_ifinfo(). Back to your question - > it's only the new name. So the program needs to keep track of ifindex to know which interface has changed name. Doable. I still expect this has the potential to break something. You probably should be asking on linux-api for the API experts opinion. Andrew
Andrew Lunn <andrew@lunn.ch> writes: > On Thu, Aug 10, 2017 at 05:24:55PM +0200, Vitaly Kuznetsov wrote: >> Andrew Lunn <andrew@lunn.ch> writes: >> >> >> We are - rtnetlink_event() does the job. We, however, don't have a >> >> special IFLA_EVENT_* for name change and end up with IFLA_EVENT_NONE. >> > >> > What is in this event? Old and new name? Just the new name? >> >> Basically, it's everything we know about the interface - type, index, >> name, mtu, qdisc, ... - see rtnl_fill_ifinfo(). Back to your question - >> it's only the new name. > > So the program needs to keep track of ifindex to know which interface > has changed name. Doable. > Yes, and I'd expect that's what these daemons do nowdays to track name changes for down interfaces (if/when they care). > I still expect this has the potential to break something. You probably > should be asking on linux-api for the API experts opinion. > Good idea, I'll do RFCv2 submission.
From: Andrew Lunn <andrew@lunn.ch> Date: Thu, 10 Aug 2017 18:27:22 +0200 > On Thu, Aug 10, 2017 at 05:24:55PM +0200, Vitaly Kuznetsov wrote: >> Andrew Lunn <andrew@lunn.ch> writes: >> >> >> We are - rtnetlink_event() does the job. We, however, don't have a >> >> special IFLA_EVENT_* for name change and end up with IFLA_EVENT_NONE. >> > >> > What is in this event? Old and new name? Just the new name? >> >> Basically, it's everything we know about the interface - type, index, >> name, mtu, qdisc, ... - see rtnl_fill_ifinfo(). Back to your question - >> it's only the new name. > > So the program needs to keep track of ifindex to know which interface > has changed name. Doable. > > I still expect this has the potential to break something. You probably > should be asking on linux-api for the API experts opinion. But a greater point is that nobody is monitoring device renames explicitly right now. It's hard to legitimize imposing new requirements like this on daemons which didn't exist before. If they could assume the name didn't change, they probably are doing so. You can't expect that just because they might already be listening to this event for device up and down tracking that they are handling device renames.
On 8/10/17 10:48 AM, David Miller wrote: > From: Andrew Lunn <andrew@lunn.ch> > Date: Thu, 10 Aug 2017 18:27:22 +0200 > >> On Thu, Aug 10, 2017 at 05:24:55PM +0200, Vitaly Kuznetsov wrote: >>> Andrew Lunn <andrew@lunn.ch> writes: >>> >>>>> We are - rtnetlink_event() does the job. We, however, don't have a >>>>> special IFLA_EVENT_* for name change and end up with IFLA_EVENT_NONE. >>>> >>>> What is in this event? Old and new name? Just the new name? >>> >>> Basically, it's everything we know about the interface - type, index, >>> name, mtu, qdisc, ... - see rtnl_fill_ifinfo(). Back to your question - >>> it's only the new name. >> >> So the program needs to keep track of ifindex to know which interface >> has changed name. Doable. >> >> I still expect this has the potential to break something. You probably >> should be asking on linux-api for the API experts opinion. > > But a greater point is that nobody is monitoring device renames > explicitly right now. Just to throw in an example: https://github.com/kobolabs/dhcpcd/blob/kobo/if-linux.c#L761 Learned of its use from a recent regression: https://bugzilla.kernel.org/show_bug.cgi?id=196355
On Thu, 10 Aug 2017 10:55:01 -0600 David Ahern <dsahern@gmail.com> wrote: > On 8/10/17 10:48 AM, David Miller wrote: > > From: Andrew Lunn <andrew@lunn.ch> > > Date: Thu, 10 Aug 2017 18:27:22 +0200 > > > >> On Thu, Aug 10, 2017 at 05:24:55PM +0200, Vitaly Kuznetsov wrote: > >>> Andrew Lunn <andrew@lunn.ch> writes: > >>> > >>>>> We are - rtnetlink_event() does the job. We, however, don't have a > >>>>> special IFLA_EVENT_* for name change and end up with IFLA_EVENT_NONE. > >>>> > >>>> What is in this event? Old and new name? Just the new name? > >>> > >>> Basically, it's everything we know about the interface - type, index, > >>> name, mtu, qdisc, ... - see rtnl_fill_ifinfo(). Back to your question - > >>> it's only the new name. > >> > >> So the program needs to keep track of ifindex to know which interface > >> has changed name. Doable. > >> > >> I still expect this has the potential to break something. You probably > >> should be asking on linux-api for the API experts opinion. > > > > But a greater point is that nobody is monitoring device renames > > explicitly right now. > > Just to throw in an example: > https://github.com/kobolabs/dhcpcd/blob/kobo/if-linux.c#L761 > > Learned of its use from a recent regression: > https://bugzilla.kernel.org/show_bug.cgi?id=196355 Quagga is another example of what might break. Especially with all the new forks..
Stephen Hemminger <stephen@networkplumber.org> writes: > On Thu, 10 Aug 2017 10:55:01 -0600 > David Ahern <dsahern@gmail.com> wrote: > >> On 8/10/17 10:48 AM, David Miller wrote: >> > From: Andrew Lunn <andrew@lunn.ch> >> > Date: Thu, 10 Aug 2017 18:27:22 +0200 >> > >> >> On Thu, Aug 10, 2017 at 05:24:55PM +0200, Vitaly Kuznetsov wrote: >> >>> Andrew Lunn <andrew@lunn.ch> writes: >> >>> >> >>>>> We are - rtnetlink_event() does the job. We, however, don't have a >> >>>>> special IFLA_EVENT_* for name change and end up with IFLA_EVENT_NONE. >> >>>> >> >>>> What is in this event? Old and new name? Just the new name? >> >>> >> >>> Basically, it's everything we know about the interface - type, index, >> >>> name, mtu, qdisc, ... - see rtnl_fill_ifinfo(). Back to your question - >> >>> it's only the new name. >> >> >> >> So the program needs to keep track of ifindex to know which interface >> >> has changed name. Doable. >> >> >> >> I still expect this has the potential to break something. You probably >> >> should be asking on linux-api for the API experts opinion. >> > >> > But a greater point is that nobody is monitoring device renames >> > explicitly right now. >> >> Just to throw in an example: >> https://github.com/kobolabs/dhcpcd/blob/kobo/if-linux.c#L761 >> >> Learned of its use from a recent regression: >> https://bugzilla.kernel.org/show_bug.cgi?id=196355 > > Quagga is another example of what might break. Especially with all the new > forks.. I see, even if we don't see right away why the limitation was imposed and who depends on this 'UP interfaces keep their names' semantics it may not be worth it to open this pandora box just because of the netvsc driver change...
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);
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(-)