| Message ID | 20210807163749.18316-1-pali@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ppp: Add rtnl attribute IFLA_PPP_UNIT_ID for specifying ppp unit id | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Sat, 7 Aug 2021 18:37:49 +0200 Pali Rohár wrote: > Currently there are two ways how to create a new ppp interface. Old method > via ioctl(PPPIOCNEWUNIT) and new method via rtnl RTM_NEWLINK/NLM_F_CREATE > which was introduced in v4.7 by commit 96d934c70db6 ("ppp: add rtnetlink > device creation support"). > > ... Your 2 previous patches were fixes and went into net, this patch seems to be on top of them but is a feature, so should go to net-next. But it doesn't apply to net-next given net was not merged into net-next. Please rebase on top of net-next or (preferably) wait until next week so that the trees can get merged and then you can repost without causing any conflicts. > static const struct nla_policy ppp_nl_policy[IFLA_PPP_MAX + 1] = { > [IFLA_PPP_DEV_FD] = { .type = NLA_S32 }, > + [IFLA_PPP_UNIT_ID] = { .type = NLA_S32 }, > }; set .strict_start_type, please so new attrs get validated better > static int ppp_nl_validate(struct nlattr *tb[], struct nlattr *data[], > @@ -1274,6 +1277,15 @@ static int ppp_nl_validate(struct nlattr *tb[], struct nlattr *data[], > > if (!data[IFLA_PPP_DEV_FD]) > return -EINVAL; > + > + /* Check for IFLA_PPP_UNIT_ID before IFLA_PPP_DEV_FD to allow userspace > + * detect if kernel supports IFLA_PPP_UNIT_ID or not by specifying > + * negative IFLA_PPP_DEV_FD. Previous kernel versions ignored > + * IFLA_PPP_UNIT_ID attribute. > + */ > + if (data[IFLA_PPP_UNIT_ID] && nla_get_s32(data[IFLA_PPP_UNIT_ID]) < -1) > + return -EINVAL; please use NLA_POLICY_MIN() instead, no need to open-code > if (nla_get_s32(data[IFLA_PPP_DEV_FD]) < 0) > return -EBADF;
On Monday 09 August 2021 12:25:46 Jakub Kicinski wrote: > On Sat, 7 Aug 2021 18:37:49 +0200 Pali Rohár wrote: > > Currently there are two ways how to create a new ppp interface. Old method > > via ioctl(PPPIOCNEWUNIT) and new method via rtnl RTM_NEWLINK/NLM_F_CREATE > > which was introduced in v4.7 by commit 96d934c70db6 ("ppp: add rtnetlink > > device creation support"). > > > > ... > > Your 2 previous patches were fixes and went into net, this patch seems > to be on top of them but is a feature, so should go to net-next. Yes > But it doesn't apply to net-next given net was not merged into net-next. > Please rebase on top of net-next or (preferably) wait until next week > so that the trees can get merged and then you can repost without causing > any conflicts. Better to wait. I would like hear some comments / review on this patch if this is the correct approach as it adds a new API/ABI for userspace. > > static const struct nla_policy ppp_nl_policy[IFLA_PPP_MAX + 1] = { > > [IFLA_PPP_DEV_FD] = { .type = NLA_S32 }, > > + [IFLA_PPP_UNIT_ID] = { .type = NLA_S32 }, > > }; > > set .strict_start_type, please so new attrs get validated better > > > static int ppp_nl_validate(struct nlattr *tb[], struct nlattr *data[], > > @@ -1274,6 +1277,15 @@ static int ppp_nl_validate(struct nlattr *tb[], struct nlattr *data[], > > > > if (!data[IFLA_PPP_DEV_FD]) > > return -EINVAL; > > + > > + /* Check for IFLA_PPP_UNIT_ID before IFLA_PPP_DEV_FD to allow userspace > > + * detect if kernel supports IFLA_PPP_UNIT_ID or not by specifying > > + * negative IFLA_PPP_DEV_FD. Previous kernel versions ignored > > + * IFLA_PPP_UNIT_ID attribute. > > + */ > > + if (data[IFLA_PPP_UNIT_ID] && nla_get_s32(data[IFLA_PPP_UNIT_ID]) < -1) > > + return -EINVAL; > > please use NLA_POLICY_MIN() instead, no need to open-code > > > if (nla_get_s32(data[IFLA_PPP_DEV_FD]) < 0) > > return -EBADF; I will look at both issues... and I would like to know what is preferred way to introduce new attributes in a way that userspace can detect if kernel supports them or not.
On Mon, Aug 09, 2021 at 09:31:09PM +0200, Pali Rohár wrote: > Better to wait. I would like hear some comments / review on this patch > if this is the correct approach as it adds a new API/ABI for userspace. Personally I don't understand the use case for setting the ppp unit at creation time. I didn't implement it on purpose when creating the netlink interface, as I didn't have any use case. On the other hand, adding the ppp unit in the netlink dump is probably useful.
On Tuesday 10 August 2021 17:39:41 Guillaume Nault wrote: > On Mon, Aug 09, 2021 at 09:31:09PM +0200, Pali Rohár wrote: > > Better to wait. I would like hear some comments / review on this patch > > if this is the correct approach as it adds a new API/ABI for userspace. > > Personally I don't understand the use case for setting the ppp unit at > creation time. I know about two use cases: * ppp unit id is used for generating network interface name. So if you want interface name ppp10 then you request for unit id 10. It is somehow common that when ppp interface has prefix "ppp" in its name then it is followed by unit id. Seems that existing ppp applications which use "ppp<num>" naming expects this. But of course you do not have to use this convention and rename interfaces as you want. * Some of ppp ioctls use unit id. So you may want to use some specific number for some network interface. So e.g. unit id 1 will be always for /dev/ttyUSB1. > I didn't implement it on purpose when creating the > netlink interface, as I didn't have any use case. > > On the other hand, adding the ppp unit in the netlink dump is probably > useful. Yes, this could be really useful as currently if you ask netlink to create a new ppp interface you have to use ioctl to retrieve this unit id. But ppp currently does not provide netlink dump operation. Also it could be useful for this "bug": https://lore.kernel.org/netdev/20210807132703.26303-1-pali@kernel.org/t/#u And with unit id there also another issue: https://lore.kernel.org/netdev/20210807160050.17687-1-pali@kernel.org/t/#u But due to how it is used we probably have to deal with it how ppp unit id are defined and assigned...
On Tuesday 10 August 2021 16:38:32 Chris Fowler wrote: > Isn't the UNIT ID the interface number? As in 'unit 100' will give me ppp100? If you do not specify pppd 'ifname' argument then pppd argument 'unit 100' will cause that interface name would be ppp100. But you are free to rename interface to any string which you like, even to "ppp99". But this ppp unit id is not interface number. Interface number is another number which has nothing with ppp unit id and is assigned to every network interface (even loopback). You can see them as the first number in 'ip -o l' output. Or you can retrieve it via if_nametoindex() function in C. ... So if people are really using pppd's 'unit' argument then I think it really make sense to support it also in new rtnl interface. > I have it running on over 1000 interfaces. I use PPP in a P-t-P VPN scenario between a central server and remote network devices. Iuse the unit id to identify a primary key in a database for that connection. This is calculated as 1000+N = UNIT ID. > > I use 1000 because I've also used PPP on the same system with modem boards for demand dial PPP connections. The up and down scripts executed by PPP are written in C and allow a system where if the VPN link is down, the remote can dial and obtain the same IP addressing via modem. We don't use modems that often now due to reliability issues. It has been harder obtaining clean lines in the US. > > The C program also applies routes that are defined in the database. That search is based on the IP assigned, not the unit id. > > Chris > > ________________________________ > From: Guillaume Nault <gnault@redhat.com> > Sent: Tuesday, August 10, 2021 11:39 AM > To: Pali Rohár <pali@kernel.org> > Cc: Jakub Kicinski <kuba@kernel.org>; Paul Mackerras <paulus@samba.org>; David S. Miller <davem@davemloft.net>; linux-ppp@vger.kernel.org <linux-ppp@vger.kernel.org>; netdev@vger.kernel.org <netdev@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH] ppp: Add rtnl attribute IFLA_PPP_UNIT_ID for specifying ppp unit id > > On Mon, Aug 09, 2021 at 09:31:09PM +0200, Pali Rohár wrote: > > Better to wait. I would like hear some comments / review on this patch > > if this is the correct approach as it adds a new API/ABI for userspace. > > Personally I don't understand the use case for setting the ppp unit at > creation time. I didn't implement it on purpose when creating the > netlink interface, as I didn't have any use case. > > On the other hand, adding the ppp unit in the netlink dump is probably > useful. >
On 8/10/21 1:16 PM, Pali Rohár wrote: > On Tuesday 10 August 2021 16:38:32 Chris Fowler wrote: >> Isn't the UNIT ID the interface number? As in 'unit 100' will give me ppp100? > > If you do not specify pppd 'ifname' argument then pppd argument 'unit 100' > will cause that interface name would be ppp100. > > But you are free to rename interface to any string which you like, even > to "ppp99". > > But this ppp unit id is not interface number. Interface number is > another number which has nothing with ppp unit id and is assigned to > every network interface (even loopback). You can see them as the first > number in 'ip -o l' output. Or you can retrieve it via if_nametoindex() > function in C. Correct; completely unrelated to the notion of "interface index." > ... So if people are really using pppd's 'unit' argument then I think it > really make sense to support it also in new rtnl interface. The pppd source base is old. It dates to the mid-80's. So it predates not just rename-able interfaces in Linux but Linux itself. I recall supported platforms in the past (BSD-derived) that didn't support allowing the user to specify the unit number. In general, on those platforms, the option was accepted and just ignored, and there were either release notes or man page updates (on that platform) that indicated that "unit N" wouldn't work there. Are there users on Linux who make use of the "unit" option and who would mourn its loss? Nobody really knows. It's an ancient feature that was originally intended to deal with systems that couldn't rename interfaces (where one had to make sure that the actual interface selected matched up with pre-configured filtering rules or static routes or the like), and to make life nice for administrators (e.g., making sure that serial port 1 maps to ppp1, port 2 is ppp2, and so on). I would think and hope most users reach for the more-flexible "ifname" option first, but I certainly can't guarantee it. It could be buried in a script somewhere or (god forbid) some kind of GUI or "usability" tool. If I were back at Sun, I'd probably call it suitable only for a "Major" release, as it removes a publicly documented feature. But I don't know what the considerations are here. Maybe it's just a "don't really care."
On Tue, Aug 10, 2021 at 06:04:50PM +0200, Pali Rohár wrote: > On Tuesday 10 August 2021 17:39:41 Guillaume Nault wrote: > > On Mon, Aug 09, 2021 at 09:31:09PM +0200, Pali Rohár wrote: > > > Better to wait. I would like hear some comments / review on this patch > > > if this is the correct approach as it adds a new API/ABI for userspace. > > > > Personally I don't understand the use case for setting the ppp unit at > > creation time. > > I know about two use cases: > > * ppp unit id is used for generating network interface name. So if you > want interface name ppp10 then you request for unit id 10. It is > somehow common that when ppp interface has prefix "ppp" in its name > then it is followed by unit id. Seems that existing ppp applications > which use "ppp<num>" naming expects this. But of course you do not > have to use this convention and rename interfaces as you want. Really, with the netlink API, the interface name has to be set with IFLA_IFNAME. There's no point in adding a new attribute just to have a side effect on the device name. > * Some of ppp ioctls use unit id. So you may want to use some specific > number for some network interface. So e.g. unit id 1 will be always > for /dev/ttyUSB1. But what's the point of forcing unit id 1 for a particular interface? One can easily get the assigned unit id with ioctl(PPPIOCGUNIT). > > I didn't implement it on purpose when creating the > > netlink interface, as I didn't have any use case. > > > > On the other hand, adding the ppp unit in the netlink dump is probably > > useful. > > Yes, this could be really useful as currently if you ask netlink to > create a new ppp interface you have to use ioctl to retrieve this unit > id. But ppp currently does not provide netlink dump operation. > > Also it could be useful for this "bug": > https://lore.kernel.org/netdev/20210807132703.26303-1-pali@kernel.org/t/#u This patch itself makes sense, but how is that related to unit id? > And with unit id there also another issue: > https://lore.kernel.org/netdev/20210807160050.17687-1-pali@kernel.org/t/#u This patch shows why linking unit id and interface name are a bad idea. Instead of adding more complexity with unit id, I'd prefer to have a new netlink attribute that says "don't generate the interface name based on the unit id". That's how the original implementation worked by the way and I'm really sad I accepted to change it... > But due to how it is used we probably have to deal with it how ppp unit > id are defined and assigned... >
On Tue, Aug 10, 2021 at 02:11:11PM -0400, James Carlson wrote: > On 8/10/21 1:16 PM, Pali Rohár wrote: > > On Tuesday 10 August 2021 16:38:32 Chris Fowler wrote: > > > Isn't the UNIT ID the interface number? As in 'unit 100' will give me ppp100? > > > > If you do not specify pppd 'ifname' argument then pppd argument 'unit 100' > > will cause that interface name would be ppp100. > > > > But you are free to rename interface to any string which you like, even > > to "ppp99". > > > > But this ppp unit id is not interface number. Interface number is > > another number which has nothing with ppp unit id and is assigned to > > every network interface (even loopback). You can see them as the first > > number in 'ip -o l' output. Or you can retrieve it via if_nametoindex() > > function in C. > > Correct; completely unrelated to the notion of "interface index." > > > ... So if people are really using pppd's 'unit' argument then I think it > > really make sense to support it also in new rtnl interface. > > The pppd source base is old. It dates to the mid-80's. So it predates not > just rename-able interfaces in Linux but Linux itself. > > I recall supported platforms in the past (BSD-derived) that didn't support > allowing the user to specify the unit number. In general, on those > platforms, the option was accepted and just ignored, and there were either > release notes or man page updates (on that platform) that indicated that > "unit N" wouldn't work there. > > Are there users on Linux who make use of the "unit" option and who would > mourn its loss? Nobody really knows. It's an ancient feature that was > originally intended to deal with systems that couldn't rename interfaces > (where one had to make sure that the actual interface selected matched up > with pre-configured filtering rules or static routes or the like), and to > make life nice for administrators (e.g., making sure that serial port 1 maps > to ppp1, port 2 is ppp2, and so on). > > I would think and hope most users reach for the more-flexible "ifname" > option first, but I certainly can't guarantee it. It could be buried in a > script somewhere or (god forbid) some kind of GUI or "usability" tool. > > If I were back at Sun, I'd probably call it suitable only for a "Major" > release, as it removes a publicly documented feature. But I don't know what > the considerations are here. Maybe it's just a "don't really care." I'm pretty sure someone, somewhere, would hate us if we broke the "unit" option. The old PPP ioctl API has been there for so long, there certainly remains tons of old tools, scripts and config files that "just work" without anybody left to debug or upgrade them. We can't just say, "starting from kernel x.y.z the unit option is a noop, use ifname instead" as affected people surely won't get the message (and there are other tools beyond pppd that may use this kernel API). But for the netlink API, we don't have to repeat the same mistake.
On Wednesday 11 August 2021 19:19:18 Guillaume Nault wrote: > On Tue, Aug 10, 2021 at 06:04:50PM +0200, Pali Rohár wrote: > > On Tuesday 10 August 2021 17:39:41 Guillaume Nault wrote: > > > On Mon, Aug 09, 2021 at 09:31:09PM +0200, Pali Rohár wrote: > > > > Better to wait. I would like hear some comments / review on this patch > > > > if this is the correct approach as it adds a new API/ABI for userspace. > > > > > > Personally I don't understand the use case for setting the ppp unit at > > > creation time. > > > > I know about two use cases: > > > > * ppp unit id is used for generating network interface name. So if you > > want interface name ppp10 then you request for unit id 10. It is > > somehow common that when ppp interface has prefix "ppp" in its name > > then it is followed by unit id. Seems that existing ppp applications > > which use "ppp<num>" naming expects this. But of course you do not > > have to use this convention and rename interfaces as you want. > > Really, with the netlink API, the interface name has to be set with > IFLA_IFNAME. There's no point in adding a new attribute just to have a > side effect on the device name. Yes, if you set IFLA_IFNAME then interface has name which you set. But if IFLA_IFNAME is not set then there is already API/ABI behavior how this interface name is generated. And all existing ppp software depends on it. > > * Some of ppp ioctls use unit id. So you may want to use some specific > > number for some network interface. So e.g. unit id 1 will be always > > for /dev/ttyUSB1. > > But what's the point of forcing unit id 1 for a particular interface? > One can easily get the assigned unit id with ioctl(PPPIOCGUNIT). Same point as ability to assign any other id to objects. It is identifier and you may want to use specific identifier for specific objects. Old ioctl API provides a way how to set this custom unit id. Why should somebody use new rtnl API if it provides only half of features? Existing software already use this feature to allow users / administrators to specify ids as they want. > > > I didn't implement it on purpose when creating the > > > netlink interface, as I didn't have any use case. > > > > > > On the other hand, adding the ppp unit in the netlink dump is probably > > > useful. > > > > Yes, this could be really useful as currently if you ask netlink to > > create a new ppp interface you have to use ioctl to retrieve this unit > > id. But ppp currently does not provide netlink dump operation. > > > > Also it could be useful for this "bug": > > https://lore.kernel.org/netdev/20210807132703.26303-1-pali@kernel.org/t/#u > > This patch itself makes sense, but how is that related to unit id? Now I see, it does not help in this unit id scenario... > > And with unit id there also another issue: > > https://lore.kernel.org/netdev/20210807160050.17687-1-pali@kernel.org/t/#u > > This patch shows why linking unit id and interface name are a bad idea. Yea... It is not a good idea, but it is how ppp is implemented in kernel since beginning. And it affects both ioctl and rtnl APIs. So we cannot do anything with it due to backward compatibility :-( > Instead of adding more complexity with unit id, I'd prefer to have a > new netlink attribute that says "don't generate the interface name > based on the unit id". That's how the original implementation worked by > the way and I'm really sad I accepted to change it... Main issue there is that kernel currently does not provide any way how to retrieve interface which was created by rtnl call. So matching interface name by string "ppp" followed by unit id is currently the only option. I must admit that ppp rtnl API was designed incorrectly. If it was able to solve this issue since beginning then this unit id <--> interface mapping did not have to been implemented in rtnl code path. But it is too late now, if rtnl API has to be backward compatible then its behavior needs to be as it is currently. > > But due to how it is used we probably have to deal with it how ppp unit > > id are defined and assigned... > > >
On Wednesday 11 August 2021 19:38:11 Guillaume Nault wrote: > On Tue, Aug 10, 2021 at 02:11:11PM -0400, James Carlson wrote: > > On 8/10/21 1:16 PM, Pali Rohár wrote: > > > On Tuesday 10 August 2021 16:38:32 Chris Fowler wrote: > > > > Isn't the UNIT ID the interface number? As in 'unit 100' will give me ppp100? > > > > > > If you do not specify pppd 'ifname' argument then pppd argument 'unit 100' > > > will cause that interface name would be ppp100. > > > > > > But you are free to rename interface to any string which you like, even > > > to "ppp99". > > > > > > But this ppp unit id is not interface number. Interface number is > > > another number which has nothing with ppp unit id and is assigned to > > > every network interface (even loopback). You can see them as the first > > > number in 'ip -o l' output. Or you can retrieve it via if_nametoindex() > > > function in C. > > > > Correct; completely unrelated to the notion of "interface index." > > > > > ... So if people are really using pppd's 'unit' argument then I think it > > > really make sense to support it also in new rtnl interface. > > > > The pppd source base is old. It dates to the mid-80's. So it predates not > > just rename-able interfaces in Linux but Linux itself. > > > > I recall supported platforms in the past (BSD-derived) that didn't support > > allowing the user to specify the unit number. In general, on those > > platforms, the option was accepted and just ignored, and there were either > > release notes or man page updates (on that platform) that indicated that > > "unit N" wouldn't work there. > > > > Are there users on Linux who make use of the "unit" option and who would > > mourn its loss? Nobody really knows. It's an ancient feature that was > > originally intended to deal with systems that couldn't rename interfaces > > (where one had to make sure that the actual interface selected matched up > > with pre-configured filtering rules or static routes or the like), and to > > make life nice for administrators (e.g., making sure that serial port 1 maps > > to ppp1, port 2 is ppp2, and so on). > > > > I would think and hope most users reach for the more-flexible "ifname" > > option first, but I certainly can't guarantee it. It could be buried in a > > script somewhere or (god forbid) some kind of GUI or "usability" tool. > > > > If I were back at Sun, I'd probably call it suitable only for a "Major" > > release, as it removes a publicly documented feature. But I don't know what > > the considerations are here. Maybe it's just a "don't really care." > > I'm pretty sure someone, somewhere, would hate us if we broke the > "unit" option. The old PPP ioctl API has been there for so long, > there certainly remains tons of old tools, scripts and config files > that "just work" without anybody left to debug or upgrade them. > > We can't just say, "starting from kernel x.y.z the unit option is a > noop, use ifname instead" as affected people surely won't get the > message (and there are other tools beyond pppd that may use this > kernel API). > > But for the netlink API, we don't have to repeat the same mistake. ifname is not atomic (first it creates ppp<id> interface and later it is renamed) and have issues. Due to bug described here: https://lore.kernel.org/netdev/20210807160050.17687-1-pali@kernel.org/ you may get your kernel into state in which it is not possible to create a new ppp interface. And this issue does not happen when using "unit" argument. To fix above issue it is needed to migrate pppd from ioctl API to rtnl. But this would be possible only after rtnl API starts providing all features, including specifying custom "unit" argument... I hit above problem, so now I'm migrating all pppd setups from "ifname" to "unit" option.
On Wed, Aug 11, 2021 at 07:54:49PM +0200, Pali Rohár wrote: > On Wednesday 11 August 2021 19:19:18 Guillaume Nault wrote: > > On Tue, Aug 10, 2021 at 06:04:50PM +0200, Pali Rohár wrote: > > > On Tuesday 10 August 2021 17:39:41 Guillaume Nault wrote: > > > > On Mon, Aug 09, 2021 at 09:31:09PM +0200, Pali Rohár wrote: > > > > > Better to wait. I would like hear some comments / review on this patch > > > > > if this is the correct approach as it adds a new API/ABI for userspace. > > > > > > > > Personally I don't understand the use case for setting the ppp unit at > > > > creation time. > > > > > > I know about two use cases: > > > > > > * ppp unit id is used for generating network interface name. So if you > > > want interface name ppp10 then you request for unit id 10. It is > > > somehow common that when ppp interface has prefix "ppp" in its name > > > then it is followed by unit id. Seems that existing ppp applications > > > which use "ppp<num>" naming expects this. But of course you do not > > > have to use this convention and rename interfaces as you want. > > > > Really, with the netlink API, the interface name has to be set with > > IFLA_IFNAME. There's no point in adding a new attribute just to have a > > side effect on the device name. > > Yes, if you set IFLA_IFNAME then interface has name which you set. But > if IFLA_IFNAME is not set then there is already API/ABI behavior how > this interface name is generated. And all existing ppp software depends > on it. They depend on the ioctl api, which is not going to change. The netlink api on the other hand is free to avoid propagating mistakes from the past. > > > * Some of ppp ioctls use unit id. So you may want to use some specific > > > number for some network interface. So e.g. unit id 1 will be always > > > for /dev/ttyUSB1. > > > > But what's the point of forcing unit id 1 for a particular interface? > > One can easily get the assigned unit id with ioctl(PPPIOCGUNIT). > > Same point as ability to assign any other id to objects. It is > identifier and you may want to use specific identifier for specific > objects. Again, what's the use case? Unit ids are kernel internal identifiers. The only purpose of setting them from user space was to influence the name of the ppp device for legacy systems that couldn't do that in a clean way. But any system with the netlink interface won't need this work around. > Old ioctl API provides a way how to set this custom unit id. Why should > somebody use new rtnl API if it provides only half of features? You still haven't provided any use case for setting the unit id in user space, appart for influencing the interface name. Netlink also allows to set the interface name and provides much more features (like creating the device in a different netns). > Existing > software already use this feature to allow users / administrators to > specify ids as they want. And that was a mistake, as you realised when working on https://lore.kernel.org/netdev/20210807160050.17687-1-pali@kernel.org/t/#u. > > > And with unit id there also another issue: > > > https://lore.kernel.org/netdev/20210807160050.17687-1-pali@kernel.org/t/#u > > > > This patch shows why linking unit id and interface name are a bad idea. > > Yea... It is not a good idea, but it is how ppp is implemented in > kernel since beginning. And it affects both ioctl and rtnl APIs. So we > cannot do anything with it due to backward compatibility :-( Sorry, but I still hardly see the problem with the netlink api. I shouldn't have accepted to let the unit id influence the interface name, true. But that doesn't seem to be what you're complaining about. Also, it could be useful to add the unit id in netlink dumps. But we already agreed on that. > > Instead of adding more complexity with unit id, I'd prefer to have a > > new netlink attribute that says "don't generate the interface name > > based on the unit id". That's how the original implementation worked by > > the way and I'm really sad I accepted to change it... > > Main issue there is that kernel currently does not provide any way how > to retrieve interface which was created by rtnl call. So matching > interface name by string "ppp" followed by unit id is currently the only > option. Yes, that's an old limitation of rtnl. But it's a much more general problem. A work around is to set the interface name in the netlink request. I can't see how forcing the unit id could ever help. > I must admit that ppp rtnl API was designed incorrectly. If it was able > to solve this issue since beginning then this unit id <--> interface > mapping did not have to been implemented in rtnl code path. As I already proposed, we can add an attribute to make the interface name independant from the unit id. > But it is too late now, if rtnl API has to be backward compatible then > its behavior needs to be as it is currently. Adding a new attribute is always possible. > > > But due to how it is used we probably have to deal with it how ppp unit > > > id are defined and assigned... > > > > > >
On Wed, Aug 11, 2021 at 08:04:01PM +0200, Pali Rohár wrote: > On Wednesday 11 August 2021 19:38:11 Guillaume Nault wrote: > > On Tue, Aug 10, 2021 at 02:11:11PM -0400, James Carlson wrote: > > > On 8/10/21 1:16 PM, Pali Rohár wrote: > > > > On Tuesday 10 August 2021 16:38:32 Chris Fowler wrote: > > > > > Isn't the UNIT ID the interface number? As in 'unit 100' will give me ppp100? > > > > > > > > If you do not specify pppd 'ifname' argument then pppd argument 'unit 100' > > > > will cause that interface name would be ppp100. > > > > > > > > But you are free to rename interface to any string which you like, even > > > > to "ppp99". > > > > > > > > But this ppp unit id is not interface number. Interface number is > > > > another number which has nothing with ppp unit id and is assigned to > > > > every network interface (even loopback). You can see them as the first > > > > number in 'ip -o l' output. Or you can retrieve it via if_nametoindex() > > > > function in C. > > > > > > Correct; completely unrelated to the notion of "interface index." > > > > > > > ... So if people are really using pppd's 'unit' argument then I think it > > > > really make sense to support it also in new rtnl interface. > > > > > > The pppd source base is old. It dates to the mid-80's. So it predates not > > > just rename-able interfaces in Linux but Linux itself. > > > > > > I recall supported platforms in the past (BSD-derived) that didn't support > > > allowing the user to specify the unit number. In general, on those > > > platforms, the option was accepted and just ignored, and there were either > > > release notes or man page updates (on that platform) that indicated that > > > "unit N" wouldn't work there. > > > > > > Are there users on Linux who make use of the "unit" option and who would > > > mourn its loss? Nobody really knows. It's an ancient feature that was > > > originally intended to deal with systems that couldn't rename interfaces > > > (where one had to make sure that the actual interface selected matched up > > > with pre-configured filtering rules or static routes or the like), and to > > > make life nice for administrators (e.g., making sure that serial port 1 maps > > > to ppp1, port 2 is ppp2, and so on). > > > > > > I would think and hope most users reach for the more-flexible "ifname" > > > option first, but I certainly can't guarantee it. It could be buried in a > > > script somewhere or (god forbid) some kind of GUI or "usability" tool. > > > > > > If I were back at Sun, I'd probably call it suitable only for a "Major" > > > release, as it removes a publicly documented feature. But I don't know what > > > the considerations are here. Maybe it's just a "don't really care." > > > > I'm pretty sure someone, somewhere, would hate us if we broke the > > "unit" option. The old PPP ioctl API has been there for so long, > > there certainly remains tons of old tools, scripts and config files > > that "just work" without anybody left to debug or upgrade them. > > > > We can't just say, "starting from kernel x.y.z the unit option is a > > noop, use ifname instead" as affected people surely won't get the > > message (and there are other tools beyond pppd that may use this > > kernel API). > > > > But for the netlink API, we don't have to repeat the same mistake. > > ifname is not atomic (first it creates ppp<id> interface and later it is > renamed) and have issues. Due to bug described here: > https://lore.kernel.org/netdev/20210807160050.17687-1-pali@kernel.org/ > you may get your kernel into state in which it is not possible to create > a new ppp interface. And this issue does not happen when using "unit" > argument. This is specific to the ioctl api. Netlink doesn't have this problem. > To fix above issue it is needed to migrate pppd from ioctl API to rtnl. It would have helped a lot if you had explained that before. > But this would be possible only after rtnl API starts providing all > features, including specifying custom "unit" argument... You can already simulate the "unit" option by setting the interface name as "ppp${unit}" and retrieving the kernel assigned id with ioctl(PPPIOCGUNIT). What's wrong with that? > I hit above problem, so now I'm migrating all pppd setups from "ifname" > to "unit" option. Why did you write 3125f26c51482 ("ppp: Fix generating ppp unit id when ifname is not specified") then?
On Thursday 12 August 2021 11:28:47 Guillaume Nault wrote: > On Wed, Aug 11, 2021 at 08:04:01PM +0200, Pali Rohár wrote: > > On Wednesday 11 August 2021 19:38:11 Guillaume Nault wrote: > > > On Tue, Aug 10, 2021 at 02:11:11PM -0400, James Carlson wrote: > > > > On 8/10/21 1:16 PM, Pali Rohár wrote: > > > > > On Tuesday 10 August 2021 16:38:32 Chris Fowler wrote: > > > > > > Isn't the UNIT ID the interface number? As in 'unit 100' will give me ppp100? > > > > > > > > > > If you do not specify pppd 'ifname' argument then pppd argument 'unit 100' > > > > > will cause that interface name would be ppp100. > > > > > > > > > > But you are free to rename interface to any string which you like, even > > > > > to "ppp99". > > > > > > > > > > But this ppp unit id is not interface number. Interface number is > > > > > another number which has nothing with ppp unit id and is assigned to > > > > > every network interface (even loopback). You can see them as the first > > > > > number in 'ip -o l' output. Or you can retrieve it via if_nametoindex() > > > > > function in C. > > > > > > > > Correct; completely unrelated to the notion of "interface index." > > > > > > > > > ... So if people are really using pppd's 'unit' argument then I think it > > > > > really make sense to support it also in new rtnl interface. > > > > > > > > The pppd source base is old. It dates to the mid-80's. So it predates not > > > > just rename-able interfaces in Linux but Linux itself. > > > > > > > > I recall supported platforms in the past (BSD-derived) that didn't support > > > > allowing the user to specify the unit number. In general, on those > > > > platforms, the option was accepted and just ignored, and there were either > > > > release notes or man page updates (on that platform) that indicated that > > > > "unit N" wouldn't work there. > > > > > > > > Are there users on Linux who make use of the "unit" option and who would > > > > mourn its loss? Nobody really knows. It's an ancient feature that was > > > > originally intended to deal with systems that couldn't rename interfaces > > > > (where one had to make sure that the actual interface selected matched up > > > > with pre-configured filtering rules or static routes or the like), and to > > > > make life nice for administrators (e.g., making sure that serial port 1 maps > > > > to ppp1, port 2 is ppp2, and so on). > > > > > > > > I would think and hope most users reach for the more-flexible "ifname" > > > > option first, but I certainly can't guarantee it. It could be buried in a > > > > script somewhere or (god forbid) some kind of GUI or "usability" tool. > > > > > > > > If I were back at Sun, I'd probably call it suitable only for a "Major" > > > > release, as it removes a publicly documented feature. But I don't know what > > > > the considerations are here. Maybe it's just a "don't really care." > > > > > > I'm pretty sure someone, somewhere, would hate us if we broke the > > > "unit" option. The old PPP ioctl API has been there for so long, > > > there certainly remains tons of old tools, scripts and config files > > > that "just work" without anybody left to debug or upgrade them. > > > > > > We can't just say, "starting from kernel x.y.z the unit option is a > > > noop, use ifname instead" as affected people surely won't get the > > > message (and there are other tools beyond pppd that may use this > > > kernel API). > > > > > > But for the netlink API, we don't have to repeat the same mistake. > > > > ifname is not atomic (first it creates ppp<id> interface and later it is > > renamed) and have issues. Due to bug described here: > > https://lore.kernel.org/netdev/20210807160050.17687-1-pali@kernel.org/ > > you may get your kernel into state in which it is not possible to create > > a new ppp interface. And this issue does not happen when using "unit" > > argument. > > This is specific to the ioctl api. Netlink doesn't have this problem. netlink does not have problem with implementing ifname option atomically. That is why I started looking at netlink how to avoid problems with renaming. As on some systems I see that some udev rules or NetworkManager tries to query newly created interfaces, but based on name (not id). So early renaming cause issues to these tools... But netlink is affected by above bug when "ifname" is not specified. > > To fix above issue it is needed to migrate pppd from ioctl API to rtnl. > > It would have helped a lot if you had explained that before. > > > But this would be possible only after rtnl API starts providing all > > features, including specifying custom "unit" argument... > > You can already simulate the "unit" option by setting the interface > name as "ppp${unit}" and retrieving the kernel assigned id with > ioctl(PPPIOCGUNIT). What's wrong with that? This is possible to implement. But then unit part from "ppp${unit}" would not match PPPIOCGUNIT number - like it is currently. And it is something which applications expect. Basically there is no difference between ppp interface created by ioctl and ppp interface created by rtnl. You can use other rtnl commands on ppp interface created by ioctl and also you can use other ppp ioctls on ppp interface created by rtnl. But I understand your arguments. You are looking at ppp unit id as some internal kernel number; which should probably stay in kernel. My point of view is that this is legacy identifier bound to the every ppp network interface, and which is exported to userspace. And because there is API for userspace how userspace can force particular id for particular ppp interface, it means that userspace have full control how these ids are generated. Even it is "internal" kernel number. And it does not matter how are ppp interfaces created, via which method. It is bounded to every ppp interface independently how ppp was created. By this design, userspace application may choose to create mapping between /dev/ttyUSB<N> and ppp unit <id> by having <N> == <id>. This ppp unit id is used for some operations, so it is required to know it. And if application is doing e.g. above assumption (it does not use PPPIOCGUNIT, but derive ppp unit id from /dev/ttyUSB* name) which current ioctl API allows, then this application cannot be migrated from ioctl to rtnl API without rewriting code which uses above assumption. I'm not saying if this is a good or bad idea, just I'm describing what ioctl API allows and what does not. (And yes, in my opinion it is a bad idea, but ppp is designed to allow it). If I was designing ppp again, I would have probably used interface id as ppp unit id... > > I hit above problem, so now I'm migrating all pppd setups from "ifname" > > to "unit" option. > > Why did you write 3125f26c51482 ("ppp: Fix generating ppp unit id when > ifname is not specified") then? Well, I hope that this kernel fix propagates into kernels used on affected machines. But it will take some time. And until it happens this migration is needed. Lets say it is workaround for unspecific time period.
On Thursday 12 August 2021 11:19:41 Guillaume Nault wrote: > On Wed, Aug 11, 2021 at 07:54:49PM +0200, Pali Rohár wrote: > > On Wednesday 11 August 2021 19:19:18 Guillaume Nault wrote: > > > On Tue, Aug 10, 2021 at 06:04:50PM +0200, Pali Rohár wrote: > > > > On Tuesday 10 August 2021 17:39:41 Guillaume Nault wrote: > > > > > On Mon, Aug 09, 2021 at 09:31:09PM +0200, Pali Rohár wrote: > > > > > > Better to wait. I would like hear some comments / review on this patch > > > > > > if this is the correct approach as it adds a new API/ABI for userspace. > > > > > > > > > > Personally I don't understand the use case for setting the ppp unit at > > > > > creation time. > > > > > > > > I know about two use cases: > > > > > > > > * ppp unit id is used for generating network interface name. So if you > > > > want interface name ppp10 then you request for unit id 10. It is > > > > somehow common that when ppp interface has prefix "ppp" in its name > > > > then it is followed by unit id. Seems that existing ppp applications > > > > which use "ppp<num>" naming expects this. But of course you do not > > > > have to use this convention and rename interfaces as you want. > > > > > > Really, with the netlink API, the interface name has to be set with > > > IFLA_IFNAME. There's no point in adding a new attribute just to have a > > > side effect on the device name. > > > > Yes, if you set IFLA_IFNAME then interface has name which you set. But > > if IFLA_IFNAME is not set then there is already API/ABI behavior how > > this interface name is generated. And all existing ppp software depends > > on it. > > They depend on the ioctl api, which is not going to change. > The netlink api on the other hand is free to avoid propagating mistakes > from the past. > > > > > * Some of ppp ioctls use unit id. So you may want to use some specific > > > > number for some network interface. So e.g. unit id 1 will be always > > > > for /dev/ttyUSB1. > > > > > > But what's the point of forcing unit id 1 for a particular interface? > > > One can easily get the assigned unit id with ioctl(PPPIOCGUNIT). > > > > Same point as ability to assign any other id to objects. It is > > identifier and you may want to use specific identifier for specific > > objects. > > Again, what's the use case? Unit ids are kernel internal identifiers. > The only purpose of setting them from user space was to influence the > name of the ppp device for legacy systems that couldn't do that in a > clean way. But any system with the netlink interface won't need this > work around. > > > Old ioctl API provides a way how to set this custom unit id. Why should > > somebody use new rtnl API if it provides only half of features? > > You still haven't provided any use case for setting the unit id in user > space, appart for influencing the interface name. Netlink also allows > to set the interface name and provides much more features (like > creating the device in a different netns). > > > Existing > > software already use this feature to allow users / administrators to > > specify ids as they want. > > And that was a mistake, as you realised when working on > https://lore.kernel.org/netdev/20210807160050.17687-1-pali@kernel.org/t/#u. > > > > > And with unit id there also another issue: > > > > https://lore.kernel.org/netdev/20210807160050.17687-1-pali@kernel.org/t/#u > > > > > > This patch shows why linking unit id and interface name are a bad idea. > > > > Yea... It is not a good idea, but it is how ppp is implemented in > > kernel since beginning. And it affects both ioctl and rtnl APIs. So we > > cannot do anything with it due to backward compatibility :-( > > Sorry, but I still hardly see the problem with the netlink api. The problem is that ppp from rtnl is of the same class as ppp from ioctl. And if you want to use ppp, you still have to use lot of ioctl calls as rtnl does not implement them. And these ioctl calls use ppp unit id, not interface id / interface name. So in the end you can use RTM_NEWLINK and then control ppp via ioctls. And for controlling you have to known that ppp unit id. If you are using ppp over serial devices, you can "simplify" it by forcing mapping that serial number device matches ppp unit id. And then you do not have to use dynamic ids (and need for call PPPIOCGUNIT). With dynamic unit id allocation (which is currently the only option when creating ppp via rtnl) for single ppp connection you need to know: * id of serial tty device * id of channel bound to tty device * id of network interface * id of ppp unit bound to network interface > I shouldn't have accepted to let the unit id influence the interface > name, true. I agree here. But it is too late. > But that doesn't seem to be what you're complaining about. > Also, it could be useful to add the unit id in netlink dumps. But we > already agreed on that. Yes! > > > Instead of adding more complexity with unit id, I'd prefer to have a > > > new netlink attribute that says "don't generate the interface name > > > based on the unit id". That's how the original implementation worked by > > > the way and I'm really sad I accepted to change it... > > > > Main issue there is that kernel currently does not provide any way how > > to retrieve interface which was created by rtnl call. So matching > > interface name by string "ppp" followed by unit id is currently the only > > option. > > Yes, that's an old limitation of rtnl. But it's a much more general > problem. A work around is to set the interface name in the netlink > request. I can't see how forcing the unit id could ever help. > > > I must admit that ppp rtnl API was designed incorrectly. If it was able > > to solve this issue since beginning then this unit id <--> interface > > mapping did not have to been implemented in rtnl code path. > > As I already proposed, we can add an attribute to make the interface > name independant from the unit id. > > > But it is too late now, if rtnl API has to be backward compatible then > > its behavior needs to be as it is currently. > > Adding a new attribute is always possible. I agree, that above proposal with a new attribute which makes interface name independent from the ppp unit id is a good idea. Probably it should have been default rtnl behavior (but now it is too late for changing default behavior). But prior adding this attribute, we first need a way how to retrieve interface name of newly created interface. Which we agreed that NLM_F_ECHO for RTM_NEWLINK/NLM_F_CREATE is needed. > > > > But due to how it is used we probably have to deal with it how ppp unit > > > > id are defined and assigned... > > > > > > > > > >
On Thu, Aug 12, 2021 at 03:48:45PM +0200, Pali Rohár wrote: > On Thursday 12 August 2021 11:28:47 Guillaume Nault wrote: > > On Wed, Aug 11, 2021 at 08:04:01PM +0200, Pali Rohár wrote: > > > ifname is not atomic (first it creates ppp<id> interface and later it is > > > renamed) and have issues. Due to bug described here: > > > https://lore.kernel.org/netdev/20210807160050.17687-1-pali@kernel.org/ > > > you may get your kernel into state in which it is not possible to create > > > a new ppp interface. And this issue does not happen when using "unit" > > > argument. > > > > This is specific to the ioctl api. Netlink doesn't have this problem. > > netlink does not have problem with implementing ifname option > atomically. That is why I started looking at netlink how to avoid > problems with renaming. As on some systems I see that some udev rules or > NetworkManager tries to query newly created interfaces, but based on > name (not id). So early renaming cause issues to these tools... > > But netlink is affected by above bug when "ifname" is not specified. As disscussed in another part of the thread, let's fix that with a new netlink attribute. > > > To fix above issue it is needed to migrate pppd from ioctl API to rtnl. > > > > It would have helped a lot if you had explained that before. > > > > > But this would be possible only after rtnl API starts providing all > > > features, including specifying custom "unit" argument... > > > > You can already simulate the "unit" option by setting the interface > > name as "ppp${unit}" and retrieving the kernel assigned id with > > ioctl(PPPIOCGUNIT). What's wrong with that? > > This is possible to implement. But then unit part from "ppp${unit}" > would not match PPPIOCGUNIT number - like it is currently. And it is > something which applications expect. Basically there is no difference > between ppp interface created by ioctl and ppp interface created by > rtnl. You can use other rtnl commands on ppp interface created by ioctl > and also you can use other ppp ioctls on ppp interface created by rtnl. But the application knows if it created the ppp device with a specified unit id or not. So it knows if an ioctl(PPPIOCGUNIT) call is necessary to get the unit id. And if we allow the interface name to be unrelated to the unit id, the application will also know that, because it explicitely requested it. > But I understand your arguments. You are looking at ppp unit id as some > internal kernel number; which should probably stay in kernel. Well, it has to be exported, but it should be opaque to user space (appart from the ioctl() api which is established behaviour). > My point of view is that this is legacy identifier bound to the every > ppp network interface, and which is exported to userspace. And because > there is API for userspace how userspace can force particular id for > particular ppp interface, it means that userspace have full control how > these ids are generated. Even it is "internal" kernel number. And it > does not matter how are ppp interfaces created, via which method. It is > bounded to every ppp interface independently how ppp was created. > > By this design, userspace application may choose to create mapping > between /dev/ttyUSB<N> and ppp unit <id> by having <N> == <id>. > > This ppp unit id is used for some operations, so it is required to know > it. And if application is doing e.g. above assumption (it does not use > PPPIOCGUNIT, but derive ppp unit id from /dev/ttyUSB* name) which > current ioctl API allows, then this application cannot be migrated from > ioctl to rtnl API without rewriting code which uses above assumption. Migrating such application requires writing the netlink code for the new api. How could a simple ioctl(PPPIOCGUNIT) call prevent such migration? BTW, using PPPIOCGUNIT is much cleaner an more robust that parsing the device name, so it's a win in any case. And the application is still able to name the ppp interface ppp<N> to keep things simple for its users. > I'm not saying if this is a good or bad idea, just I'm describing what > ioctl API allows and what does not. (And yes, in my opinion it is a bad > idea, but ppp is designed to allow it). > > If I was designing ppp again, I would have probably used interface id as > ppp unit id... With all the building blocks we have now in the Linux kernel, there's much more that I'd change. But the landscape and constraints were obviously very different at the time. > > > I hit above problem, so now I'm migrating all pppd setups from "ifname" > > > to "unit" option. > > > > Why did you write 3125f26c51482 ("ppp: Fix generating ppp unit id when > > ifname is not specified") then? > > Well, I hope that this kernel fix propagates into kernels used on > affected machines. But it will take some time. And until it happens this > migration is needed. Lets say it is workaround for unspecific time > period. Makes sense.
On Thursday 12 August 2021 20:26:45 Guillaume Nault wrote: > On Thu, Aug 12, 2021 at 03:48:45PM +0200, Pali Rohár wrote: > > On Thursday 12 August 2021 11:28:47 Guillaume Nault wrote: > > > On Wed, Aug 11, 2021 at 08:04:01PM +0200, Pali Rohár wrote: > > > > ifname is not atomic (first it creates ppp<id> interface and later it is > > > > renamed) and have issues. Due to bug described here: > > > > https://lore.kernel.org/netdev/20210807160050.17687-1-pali@kernel.org/ > > > > you may get your kernel into state in which it is not possible to create > > > > a new ppp interface. And this issue does not happen when using "unit" > > > > argument. > > > > > > This is specific to the ioctl api. Netlink doesn't have this problem. > > > > netlink does not have problem with implementing ifname option > > atomically. That is why I started looking at netlink how to avoid > > problems with renaming. As on some systems I see that some udev rules or > > NetworkManager tries to query newly created interfaces, but based on > > name (not id). So early renaming cause issues to these tools... > > > > But netlink is affected by above bug when "ifname" is not specified. > > As disscussed in another part of the thread, let's fix that with a new > netlink attribute. +1 > > > > To fix above issue it is needed to migrate pppd from ioctl API to rtnl. > > > > > > It would have helped a lot if you had explained that before. > > > > > > > But this would be possible only after rtnl API starts providing all > > > > features, including specifying custom "unit" argument... > > > > > > You can already simulate the "unit" option by setting the interface > > > name as "ppp${unit}" and retrieving the kernel assigned id with > > > ioctl(PPPIOCGUNIT). What's wrong with that? > > > > This is possible to implement. But then unit part from "ppp${unit}" > > would not match PPPIOCGUNIT number - like it is currently. And it is > > something which applications expect. Basically there is no difference > > between ppp interface created by ioctl and ppp interface created by > > rtnl. You can use other rtnl commands on ppp interface created by ioctl > > and also you can use other ppp ioctls on ppp interface created by rtnl. > > But the application knows if it created the ppp device with a specified > unit id or not. So it knows if an ioctl(PPPIOCGUNIT) call is necessary > to get the unit id. And if we allow the interface name to be unrelated > to the unit id, the application will also know that, because it > explicitely requested it. > > > But I understand your arguments. You are looking at ppp unit id as some > > internal kernel number; which should probably stay in kernel. > > Well, it has to be exported, but it should be opaque to user space > (appart from the ioctl() api which is established behaviour). > > > My point of view is that this is legacy identifier bound to the every > > ppp network interface, and which is exported to userspace. And because > > there is API for userspace how userspace can force particular id for > > particular ppp interface, it means that userspace have full control how > > these ids are generated. Even it is "internal" kernel number. And it > > does not matter how are ppp interfaces created, via which method. It is > > bounded to every ppp interface independently how ppp was created. > > > > By this design, userspace application may choose to create mapping > > between /dev/ttyUSB<N> and ppp unit <id> by having <N> == <id>. > > > > This ppp unit id is used for some operations, so it is required to know > > it. And if application is doing e.g. above assumption (it does not use > > PPPIOCGUNIT, but derive ppp unit id from /dev/ttyUSB* name) which > > current ioctl API allows, then this application cannot be migrated from > > ioctl to rtnl API without rewriting code which uses above assumption. > > Migrating such application requires writing the netlink code for the new > api. How could a simple ioctl(PPPIOCGUNIT) call prevent such migration? > BTW, using PPPIOCGUNIT is much cleaner an more robust that parsing the > device name, so it's a win in any case. And the application is still > able to name the ppp interface ppp<N> to keep things simple for its > users. You are right and I agree with you that application with PPPIOCGUNIT is more robust. The point here is that there is application (pppd) which allows specifying custom unit id as an option argument. Also it allows to call external applications (at some events) with sharing file descriptors. And it is one of the options how to touch part of ppp connection via external scripts / applications. You start pppd for /dev/ttyUSB<N> with unit id <N> and then in external script you use <N> for ioctls. And I do not know if there is a way how to retrieve unit id in those external scripts. There was already discussion about marking all file descriptors in pppd as close-on-exec and it was somehow rejected as it will broke custom scripts / applications which pppd invokes on events. So looks like that people are using these "features" of pppd. Option "unit" in pppd specifies ppp unit id. And if new API (rtnl) would not provide equivalent for allowing to specify it then migrating pppd from ioctl to rtnl is not possible without breaking compatibility. As you already described, we can simulate setting default interface name in pppd application. But above usage or any other which expose pppd API to other application is not possible to simulate. So I think we need to first decide or solve issue if rtnl ppp API should provide same functionality as ioctl ppp API. If answer is yes, then some kind of specifying custom ppp unit id is required. If answer is no (e.g. because we do not want ppp unit id in rtnl API as it looks legacy and has issues) then rtnl ppp API cannot be used by ppp as it cannot provide all existing / supported features without breaking legacy compatibility. I see pros & cons for both answers. Not supporting legacy code paths in new code/API is the way how to clean up code and prevent repeating old historic issues. But if new code/API is not fully suitable for pppd -- which is de-facto standard Linux userspace implementation -- does it make sense to have it? Or does it mean to also implement new userspace part of implementation (e.g. pppd2) to avoid these legacy / historic issues? Or... is not whole ppp protocol just legacy part of our history which should not be used in new modern setups? And for "legacy usage" is current implementation enough and it does not make sense to invest time into this area? I cannot answer to these questions, but I think it is something quite important as it can show what should be direction and future of ppp subsystem. > > I'm not saying if this is a good or bad idea, just I'm describing what > > ioctl API allows and what does not. (And yes, in my opinion it is a bad > > idea, but ppp is designed to allow it). > > > > If I was designing ppp again, I would have probably used interface id as > > ppp unit id... > > With all the building blocks we have now in the Linux kernel, there's > much more that I'd change. But the landscape and constraints were > obviously very different at the time. > > > > > I hit above problem, so now I'm migrating all pppd setups from "ifname" > > > > to "unit" option. > > > > > > Why did you write 3125f26c51482 ("ppp: Fix generating ppp unit id when > > > ifname is not specified") then? > > > > Well, I hope that this kernel fix propagates into kernels used on > > affected machines. But it will take some time. And until it happens this > > migration is needed. Lets say it is workaround for unspecific time > > period. > > Makes sense. >
On Thu, Aug 12, 2021 at 04:09:18PM +0200, Pali Rohár wrote: > The problem is that ppp from rtnl is of the same class as ppp from > ioctl. And if you want to use ppp, you still have to use lot of ioctl > calls as rtnl does not implement them. And these ioctl calls use ppp > unit id, not interface id / interface name. Indeed, the netlink api only replaces ioctl(PPPIOCNEWUNIT). It's technically feasible to implement other ppp unit ioctls with netlink, but I didn't do that because: * some of them make no sense, * netlink wouldn't bring any advantage over ioctl() for these cases (and ppp is ioctl-centric anyway, so users will have to write ioctl() calls no matter what). If there was a bright future for ppp in sight, I'd certainly work on a new modern ioctl-less api. But in the current situation, that'd be just feature duplication and code churn. > So in the end you can use RTM_NEWLINK and then control ppp via ioctls. > And for controlling you have to known that ppp unit id. > > If you are using ppp over serial devices, you can "simplify" it by > forcing mapping that serial number device matches ppp unit id. And then > you do not have to use dynamic ids (and need for call PPPIOCGUNIT). How is dropping a single PPPIOCGUNIT call going to simplify the code while you have to write netlink message handlers? > With dynamic unit id allocation (which is currently the only option when > creating ppp via rtnl) for single ppp connection you need to know: > * id of serial tty device > * id of channel bound to tty device > * id of network interface > * id of ppp unit bound to network interface I see you're not working with L2TPv2 :). A few more things you'd need to add to the list: * a UDP socket, * a tunnel id, * a session id, * a tunnel file descriptor, * a session file descriptor, * a new ioctl() to figure out which channel id is assigned to your L2TP session, * a bunch of setsockopt() to configure the whole thing, * ... (and I'm not counting the ioctl() calls necessary to set up the channel, which also apply to your use case). Really, I'm sorry, but the possibility to drop a single PPPIOCGUNIT call isn't going to simplify the ppp landscape. > > As I already proposed, we can add an attribute to make the interface > > name independant from the unit id. > > I agree, that above proposal with a new attribute which makes interface > name independent from the ppp unit id is a good idea. Probably it should > have been default rtnl behavior (but now it is too late for changing > default behavior). Well, it was the default, until a collegue complained and I accepted to align the default device name with the original ioctl() behaviour. But the beauty of netlink is that we can revise this behaviour without breaking compatibility. > But prior adding this attribute, we first need a way how to retrieve > interface name of newly created interface. Which we agreed that > NLM_F_ECHO for RTM_NEWLINK/NLM_F_CREATE is needed. Yes, that'd be ideal. That might require quite some work though (I haven't looked in detail). At last resort, if adding NLM_F_ECHO support to rtnl proves too hard, it might be possible to add a call to get the device name associated to a ppp unit file descriptor. At least know I understand why we had this conversation about NLM_F_ECHO :).
On Thu, Aug 12, 2021 at 09:04:40PM +0200, Pali Rohár wrote: > The point here is that there is application (pppd) which allows > specifying custom unit id as an option argument. Also it allows to call > external applications (at some events) with sharing file descriptors. > And it is one of the options how to touch part of ppp connection via > external scripts / applications. You start pppd for /dev/ttyUSB<N> with > unit id <N> and then in external script you use <N> for ioctls. And I do > not know if there is a way how to retrieve unit id in those external > scripts. There was already discussion about marking all file descriptors > in pppd as close-on-exec and it was somehow rejected as it will broke > custom scripts / applications which pppd invokes on events. So looks > like that people are using these "features" of pppd. Potential external pppd scripts, that depend on the unit id, may be a valid use case for letting the netlink api define this identifier (if pppd ever gets netlink support). > Option "unit" in pppd specifies ppp unit id. And if new API (rtnl) would > not provide equivalent for allowing to specify it then migrating pppd > from ioctl to rtnl is not possible without breaking compatibility. > > As you already described, we can simulate setting default interface name > in pppd application. But above usage or any other which expose pppd API > to other application is not possible to simulate. If the pppd project is interested in adding support for the netlink api, then I'm fine with adding this feature. I just want to make sure that it'll have a real world use case. > So I think we need to first decide or solve issue if rtnl ppp API should > provide same functionality as ioctl ppp API. If answer is yes, then some > kind of specifying custom ppp unit id is required. If answer is no (e.g. > because we do not want ppp unit id in rtnl API as it looks legacy and > has issues) then rtnl ppp API cannot be used by ppp as it cannot provide > all existing / supported features without breaking legacy compatibility. > > I see pros & cons for both answers. Not supporting legacy code paths in > new code/API is the way how to clean up code and prevent repeating old > historic issues. But if new code/API is not fully suitable for pppd -- > which is de-facto standard Linux userspace implementation -- does it > make sense to have it? Or does it mean to also implement new userspace > part of implementation (e.g. pppd2) to avoid these legacy / historic > issues? Or... is not whole ppp protocol just legacy part of our history > which should not be used in new modern setups? And for "legacy usage" is > current implementation enough and it does not make sense to invest time > into this area? I cannot answer to these questions, but I think it is > something quite important as it can show what should be direction and > future of ppp subsystem. PPP isn't legacy, but very few people are interested in working on and maintaining the code. Do you have plans for adding netlink support to pppd? If so, is the project ready to accept such code? BTW, sorry for the delay.
On Monday 16 August 2021 18:11:14 Guillaume Nault wrote: > On Thu, Aug 12, 2021 at 09:04:40PM +0200, Pali Rohár wrote: > > The point here is that there is application (pppd) which allows > > specifying custom unit id as an option argument. Also it allows to call > > external applications (at some events) with sharing file descriptors. > > And it is one of the options how to touch part of ppp connection via > > external scripts / applications. You start pppd for /dev/ttyUSB<N> with > > unit id <N> and then in external script you use <N> for ioctls. And I do > > not know if there is a way how to retrieve unit id in those external > > scripts. There was already discussion about marking all file descriptors > > in pppd as close-on-exec and it was somehow rejected as it will broke > > custom scripts / applications which pppd invokes on events. So looks > > like that people are using these "features" of pppd. > > Potential external pppd scripts, that depend on the unit id, may be a > valid use case for letting the netlink api define this identifier (if > pppd ever gets netlink support). > > > Option "unit" in pppd specifies ppp unit id. And if new API (rtnl) would > > not provide equivalent for allowing to specify it then migrating pppd > > from ioctl to rtnl is not possible without breaking compatibility. > > > > As you already described, we can simulate setting default interface name > > in pppd application. But above usage or any other which expose pppd API > > to other application is not possible to simulate. > > If the pppd project is interested in adding support for the netlink > api, then I'm fine with adding this feature. I just want to make sure > that it'll have a real world use case. > > > So I think we need to first decide or solve issue if rtnl ppp API should > > provide same functionality as ioctl ppp API. If answer is yes, then some > > kind of specifying custom ppp unit id is required. If answer is no (e.g. > > because we do not want ppp unit id in rtnl API as it looks legacy and > > has issues) then rtnl ppp API cannot be used by ppp as it cannot provide > > all existing / supported features without breaking legacy compatibility. > > > > I see pros & cons for both answers. Not supporting legacy code paths in > > new code/API is the way how to clean up code and prevent repeating old > > historic issues. But if new code/API is not fully suitable for pppd -- > > which is de-facto standard Linux userspace implementation -- does it > > make sense to have it? Or does it mean to also implement new userspace > > part of implementation (e.g. pppd2) to avoid these legacy / historic > > issues? Or... is not whole ppp protocol just legacy part of our history > > which should not be used in new modern setups? And for "legacy usage" is > > current implementation enough and it does not make sense to invest time > > into this area? I cannot answer to these questions, but I think it is > > something quite important as it can show what should be direction and > > future of ppp subsystem. > > PPP isn't legacy, but very few people are interested in working on and > maintaining the code. > > Do you have plans for adding netlink support to pppd? If so, is the > project ready to accept such code? Yes, I have already some WIP code and I'm planning to send a pull request to pppd on github for it. I guess that it could be accepted, specially if there still would be backward compatibility via ioctl for kernels which do not support rtnl API. One of the argument which can be used why rtnl API is better, is fixing issue: atomic creating of interface with specific name. But pppd is maintained by Paul (already in loop), so I hope we could hear some feedback. > BTW, sorry for the delay. >
On Mon, Aug 16, 2021 at 06:23:55PM +0200, Pali Rohár wrote: > On Monday 16 August 2021 18:11:14 Guillaume Nault wrote: > > Do you have plans for adding netlink support to pppd? If so, is the > > project ready to accept such code? > > Yes, I have already some WIP code and I'm planning to send a pull > request to pppd on github for it. I guess that it could be accepted, I guess you can easily use the netlink api for cases where the "unit" option isn't specified and fall back to the ioctl api when it is. If all goes well, then we can extend the netlink api to accept a unit id. But what about the lack of netlink feedback about the created interface? Are you restricted to use netlink only when the "ifname" option is provided? > specially if there still would be backward compatibility via ioctl for > kernels which do not support rtnl API. Indeed, I'd expect keeping compatiblitity with old kernels that only have the ioctl api to be a must (but I have no experience contributing to the pppd project). > One of the argument which can be > used why rtnl API is better, is fixing issue: atomic creating of > interface with specific name. Yes, that looks useful.
On Tuesday 17 August 2021 18:05:25 Guillaume Nault wrote: > On Mon, Aug 16, 2021 at 06:23:55PM +0200, Pali Rohár wrote: > > On Monday 16 August 2021 18:11:14 Guillaume Nault wrote: > > > Do you have plans for adding netlink support to pppd? If so, is the > > > project ready to accept such code? > > > > Yes, I have already some WIP code and I'm planning to send a pull > > request to pppd on github for it. I guess that it could be accepted, > > I guess you can easily use the netlink api for cases where the "unit" > option isn't specified and fall back to the ioctl api when it is. If > all goes well, then we can extend the netlink api to accept a unit id. > > But what about the lack of netlink feedback about the created > interface? Are you restricted to use netlink only when the "ifname" > option is provided? Exactly, this is how I wrote my WIP code... > > specially if there still would be backward compatibility via ioctl for > > kernels which do not support rtnl API. > > Indeed, I'd expect keeping compatiblitity with old kernels that only > have the ioctl api to be a must (but I have no experience contributing > to the pppd project). > > > One of the argument which can be > > used why rtnl API is better, is fixing issue: atomic creating of > > interface with specific name. > > Yes, that looks useful. >
On Tuesday 17 August 2021 18:21:55 Pali Rohár wrote: > On Tuesday 17 August 2021 18:05:25 Guillaume Nault wrote: > > On Mon, Aug 16, 2021 at 06:23:55PM +0200, Pali Rohár wrote: > > > On Monday 16 August 2021 18:11:14 Guillaume Nault wrote: > > > > Do you have plans for adding netlink support to pppd? If so, is the > > > > project ready to accept such code? > > > > > > Yes, I have already some WIP code and I'm planning to send a pull > > > request to pppd on github for it. I guess that it could be accepted, > > > > I guess you can easily use the netlink api for cases where the "unit" > > option isn't specified and fall back to the ioctl api when it is. If > > all goes well, then we can extend the netlink api to accept a unit id. > > > > But what about the lack of netlink feedback about the created > > interface? Are you restricted to use netlink only when the "ifname" > > option is provided? > > Exactly, this is how I wrote my WIP code... Sorry for a long delay (I forgot about it). Now I created pull request for pppd https://github.com/ppp-project/ppp/pull/354 which adds support for creating ppp interface via rtnetlink. rtnetlink is used only when ppp unit id was not provided and interface name was provided. > > > specially if there still would be backward compatibility via ioctl for > > > kernels which do not support rtnl API. > > > > Indeed, I'd expect keeping compatiblitity with old kernels that only > > have the ioctl api to be a must (but I have no experience contributing > > to the pppd project). > > > > > One of the argument which can be > > > used why rtnl API is better, is fixing issue: atomic creating of > > > interface with specific name. > > > > Yes, that looks useful. > >
On Sat, Jul 09, 2022 at 02:09:06PM +0200, Pali Rohár wrote: > On Tuesday 17 August 2021 18:21:55 Pali Rohár wrote: > > On Tuesday 17 August 2021 18:05:25 Guillaume Nault wrote: > > > On Mon, Aug 16, 2021 at 06:23:55PM +0200, Pali Rohár wrote: > > > > On Monday 16 August 2021 18:11:14 Guillaume Nault wrote: > > > > > Do you have plans for adding netlink support to pppd? If so, is the > > > > > project ready to accept such code? > > > > > > > > Yes, I have already some WIP code and I'm planning to send a pull > > > > request to pppd on github for it. I guess that it could be accepted, > > > > > > I guess you can easily use the netlink api for cases where the "unit" > > > option isn't specified and fall back to the ioctl api when it is. If > > > all goes well, then we can extend the netlink api to accept a unit id. > > > > > > But what about the lack of netlink feedback about the created > > > interface? Are you restricted to use netlink only when the "ifname" > > > option is provided? > > > > Exactly, this is how I wrote my WIP code... > > Sorry for a long delay (I forgot about it). Now I created pull request > for pppd https://github.com/ppp-project/ppp/pull/354 which adds support > for creating ppp interface via rtnetlink. rtnetlink is used only when > ppp unit id was not provided and interface name was provided. Interesting work. Thanks for working on it! > > > > specially if there still would be backward compatibility via ioctl for > > > > kernels which do not support rtnl API. > > > > > > Indeed, I'd expect keeping compatiblitity with old kernels that only > > > have the ioctl api to be a must (but I have no experience contributing > > > to the pppd project). > > > > > > > One of the argument which can be > > > > used why rtnl API is better, is fixing issue: atomic creating of > > > > interface with specific name. > > > > > > Yes, that looks useful. > > > >
diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c index 9506abd8d7e1..bb122ff9d5cf 100644 --- a/drivers/net/ppp/ppp_generic.c +++ b/drivers/net/ppp/ppp_generic.c @@ -1147,7 +1147,7 @@ static struct pernet_operations ppp_net_ops = { .size = sizeof(struct ppp_net), }; -static int ppp_unit_register(struct ppp *ppp, int unit, bool ifname_is_set) +static int ppp_unit_register(struct ppp *ppp, int unit, bool ifname_is_set, bool rewrite_error) { struct ppp_net *pn = ppp_pernet(ppp->ppp_net); int ret; @@ -1181,8 +1181,10 @@ static int ppp_unit_register(struct ppp *ppp, int unit, bool ifname_is_set) } ret = unit_set(&pn->units_idr, ppp, unit); if (ret < 0) { - /* Rewrite error for backward compatibility */ - ret = -EEXIST; + if (rewrite_error) { + /* Rewrite error for backward compatibility */ + ret = -EEXIST; + } goto err; } } @@ -1211,7 +1213,7 @@ static int ppp_unit_register(struct ppp *ppp, int unit, bool ifname_is_set) } static int ppp_dev_configure(struct net *src_net, struct net_device *dev, - const struct ppp_config *conf) + const struct ppp_config *conf, bool rewrite_error) { struct ppp *ppp = netdev_priv(dev); int indx; @@ -1249,7 +1251,7 @@ static int ppp_dev_configure(struct net *src_net, struct net_device *dev, ppp->active_filter = NULL; #endif /* CONFIG_PPP_FILTER */ - err = ppp_unit_register(ppp, conf->unit, conf->ifname_is_set); + err = ppp_unit_register(ppp, conf->unit, conf->ifname_is_set, rewrite_error); if (err < 0) goto err2; @@ -1264,6 +1266,7 @@ static int ppp_dev_configure(struct net *src_net, struct net_device *dev, static const struct nla_policy ppp_nl_policy[IFLA_PPP_MAX + 1] = { [IFLA_PPP_DEV_FD] = { .type = NLA_S32 }, + [IFLA_PPP_UNIT_ID] = { .type = NLA_S32 }, }; static int ppp_nl_validate(struct nlattr *tb[], struct nlattr *data[], @@ -1274,6 +1277,15 @@ static int ppp_nl_validate(struct nlattr *tb[], struct nlattr *data[], if (!data[IFLA_PPP_DEV_FD]) return -EINVAL; + + /* Check for IFLA_PPP_UNIT_ID before IFLA_PPP_DEV_FD to allow userspace + * detect if kernel supports IFLA_PPP_UNIT_ID or not by specifying + * negative IFLA_PPP_DEV_FD. Previous kernel versions ignored + * IFLA_PPP_UNIT_ID attribute. + */ + if (data[IFLA_PPP_UNIT_ID] && nla_get_s32(data[IFLA_PPP_UNIT_ID]) < -1) + return -EINVAL; + if (nla_get_s32(data[IFLA_PPP_DEV_FD]) < 0) return -EBADF; @@ -1295,6 +1307,9 @@ static int ppp_nl_newlink(struct net *src_net, struct net_device *dev, if (!file) return -EBADF; + if (data[IFLA_PPP_UNIT_ID]) + conf.unit = nla_get_s32(data[IFLA_PPP_UNIT_ID]); + /* rtnl_lock is already held here, but ppp_create_interface() locks * ppp_mutex before holding rtnl_lock. Using mutex_trylock() avoids * possible deadlock due to lock order inversion, at the cost of @@ -1320,7 +1335,7 @@ static int ppp_nl_newlink(struct net *src_net, struct net_device *dev, if (!tb[IFLA_IFNAME] || !nla_len(tb[IFLA_IFNAME]) || !*(char *)nla_data(tb[IFLA_IFNAME])) conf.ifname_is_set = false; - err = ppp_dev_configure(src_net, dev, &conf); + err = ppp_dev_configure(src_net, dev, &conf, false); out_unlock: mutex_unlock(&ppp_mutex); @@ -3300,7 +3315,7 @@ static int ppp_create_interface(struct net *net, struct file *file, int *unit) rtnl_lock(); - err = ppp_dev_configure(net, dev, &conf); + err = ppp_dev_configure(net, dev, &conf, true); if (err < 0) goto err_dev; ppp = netdev_priv(dev); diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h index 91c8dda6d95d..6767bd7f39b9 100644 --- a/include/uapi/linux/if_link.h +++ b/include/uapi/linux/if_link.h @@ -794,6 +794,8 @@ enum { enum { IFLA_PPP_UNSPEC, IFLA_PPP_DEV_FD, + IFLA_PPP_UNIT_ID, +#define IFLA_PPP_UNIT_ID IFLA_PPP_UNIT_ID __IFLA_PPP_MAX }; #define IFLA_PPP_MAX (__IFLA_PPP_MAX - 1) diff --git a/tools/include/uapi/linux/if_link.h b/tools/include/uapi/linux/if_link.h index d208b2af697f..53a4d7d632f1 100644 --- a/tools/include/uapi/linux/if_link.h +++ b/tools/include/uapi/linux/if_link.h @@ -600,6 +600,8 @@ enum ifla_geneve_df { enum { IFLA_PPP_UNSPEC, IFLA_PPP_DEV_FD, + IFLA_PPP_UNIT_ID, +#define IFLA_PPP_UNIT_ID IFLA_PPP_UNIT_ID __IFLA_PPP_MAX }; #define IFLA_PPP_MAX (__IFLA_PPP_MAX - 1)
Currently there are two ways how to create a new ppp interface. Old method via ioctl(PPPIOCNEWUNIT) and new method via rtnl RTM_NEWLINK/NLM_F_CREATE which was introduced in v4.7 by commit 96d934c70db6 ("ppp: add rtnetlink device creation support"). Old method allows userspace to specify preferred ppp unit id or let kernel to choose some free ppp unit id. Newly created interface by the old method will always have name composed of string "ppp" followed by ppp unit id. Userspace later can rename interface via ioctl(SIOCSIFNAME). New method via rtnl does not allow to specify ppp unit id and kernel always choose some free one. But allows to specify interface name and therefore atomically create interface with preferred name. So based on requirement userspace needs to use either old method or new method as none currently supports all options. This change adds a new rtnl attribute IFLA_PPP_UNIT_ID which can be used for specifying preferred ppp unit id during rtnl RTM_NEWLINK/NLM_F_CREATE call. And therefore implements missing functionality which is already provided by old ioctl(PPPIOCNEWUNIT) method. By default kernel ignores unknown rtnl attributes, so userspace cannot easily check if kernel understand this new IFLA_PPP_UNIT_ID or if will ignore it. Therefore in ppp_nl_validate() first validates content of IFLA_PPP_UNIT_ID attribute and returns -EINVAL when ppp unit id is invalid. And after that validates IFLA_PPP_DEV_FD (which returns -EBADFD on error). This allows userspace to send RTM_NEWLINK/NLM_F_CREATE request with negative IFLA_PPP_DEV_FD and non-negative IFLA_PPP_UNIT_ID to detect if kernel supports IFLA_PPP_DEV_FD or not (based on -EINVAL / -EBADFD error value). Like in old ioctl(PPPIOCNEWUNIT) method treat special IFLA_PPP_UNIT_ID value -1 to let kernel choose some free ppp unit id. It is same behavior like when IFLA_PPP_UNIT_ID is not specified at all. Later userspace can use ioctl(PPPIOCGUNIT) to query which ppp unit id kernel chose. As this is a new code kernel does not have to overwrite and hide error code from ppp_unit_register() when using new rtnl method. So overwrite error code to -EEXIST only when creating a new interface via old ioctl method. With this change rtnl RTM_NEWLINK/NLM_F_CREATE can be finally full-feature replacement for the old ioctl(PPPIOCNEWUNIT) method for creating new ppp network interface. Signed-off-by: Pali Rohár <pali@kernel.org> --- drivers/net/ppp/ppp_generic.c | 29 ++++++++++++++++++++++------- include/uapi/linux/if_link.h | 2 ++ tools/include/uapi/linux/if_link.h | 2 ++ 3 files changed, 26 insertions(+), 7 deletions(-)