| Message ID | 20240604031020.2313175-1-jackie.jone@alliedtelesis.co.nz (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | igb: Add MII write support | expand |
On 6/3/2024 8:10 PM, jackie.jone@alliedtelesis.co.nz wrote: > From: Jackie Jone <jackie.jone@alliedtelesis.co.nz> > > To facilitate running PHY parametric tests, add support for the SIOCSMIIREG > ioctl. This allows a userspace application to write to the PHY registers > to enable the test modes. > > Signed-off-by: Jackie Jone <jackie.jone@alliedtelesis.co.nz> > --- > drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c > index 03a4da6a1447..7fbfcf01fbf9 100644 > --- a/drivers/net/ethernet/intel/igb/igb_main.c > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > @@ -8977,6 +8977,10 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd) > return -EIO; > break; > case SIOCSMIIREG: > + if (igb_write_phy_reg(&adapter->hw, data->reg_num & 0x1F, > + data->val_in)) > + return -EIO; > + break; A handful of drivers seem to expose this. What are the consequences of exposing this ioctl? What can user space do with it? It looks like a few drivers also check something like CAP_NET_ADMIN to avoid allowing write access to all users. Is that enforced somewhere else?
On 6/06/24 08:51, Jacob Keller wrote: > > On 6/3/2024 8:10 PM, jackie.jone@alliedtelesis.co.nz wrote: >> From: Jackie Jone <jackie.jone@alliedtelesis.co.nz> >> >> To facilitate running PHY parametric tests, add support for the SIOCSMIIREG >> ioctl. This allows a userspace application to write to the PHY registers >> to enable the test modes. >> >> Signed-off-by: Jackie Jone <jackie.jone@alliedtelesis.co.nz> >> --- >> drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c >> index 03a4da6a1447..7fbfcf01fbf9 100644 >> --- a/drivers/net/ethernet/intel/igb/igb_main.c >> +++ b/drivers/net/ethernet/intel/igb/igb_main.c >> @@ -8977,6 +8977,10 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd) >> return -EIO; >> break; >> case SIOCSMIIREG: >> + if (igb_write_phy_reg(&adapter->hw, data->reg_num & 0x1F, >> + data->val_in)) >> + return -EIO; >> + break; > A handful of drivers seem to expose this. What are the consequences of > exposing this ioctl? What can user space do with it? > > It looks like a few drivers also check something like CAP_NET_ADMIN to > avoid allowing write access to all users. Is that enforced somewhere else? CAP_NET_ADMIN is enforced via dev_ioctl() so it should already be restricted to users with that capability.
On 6/5/2024 2:10 PM, Chris Packham wrote: > > On 6/06/24 08:51, Jacob Keller wrote: >> >> On 6/3/2024 8:10 PM, jackie.jone@alliedtelesis.co.nz wrote: >>> From: Jackie Jone <jackie.jone@alliedtelesis.co.nz> >>> >>> To facilitate running PHY parametric tests, add support for the SIOCSMIIREG >>> ioctl. This allows a userspace application to write to the PHY registers >>> to enable the test modes. >>> >>> Signed-off-by: Jackie Jone <jackie.jone@alliedtelesis.co.nz> >>> --- >>> drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c >>> index 03a4da6a1447..7fbfcf01fbf9 100644 >>> --- a/drivers/net/ethernet/intel/igb/igb_main.c >>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c >>> @@ -8977,6 +8977,10 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd) >>> return -EIO; >>> break; >>> case SIOCSMIIREG: >>> + if (igb_write_phy_reg(&adapter->hw, data->reg_num & 0x1F, >>> + data->val_in)) >>> + return -EIO; >>> + break; >> A handful of drivers seem to expose this. What are the consequences of >> exposing this ioctl? What can user space do with it? >> >> It looks like a few drivers also check something like CAP_NET_ADMIN to >> avoid allowing write access to all users. Is that enforced somewhere else? > > CAP_NET_ADMIN is enforced via dev_ioctl() so it should already be > restricted to users with that capability. Ok good. That at least limits this so that random users can't cause any side effects. I'm not super familiar with what can be affected by writing the MII registers. I'm also not sure what the community thinks of exposing such access directly. From the description this is intended to use for debugging and testing purposes?
On 6/06/24 09:16, Jacob Keller wrote: > > On 6/5/2024 2:10 PM, Chris Packham wrote: >> On 6/06/24 08:51, Jacob Keller wrote: >>> On 6/3/2024 8:10 PM, jackie.jone@alliedtelesis.co.nz wrote: >>>> From: Jackie Jone <jackie.jone@alliedtelesis.co.nz> >>>> >>>> To facilitate running PHY parametric tests, add support for the SIOCSMIIREG >>>> ioctl. This allows a userspace application to write to the PHY registers >>>> to enable the test modes. >>>> >>>> Signed-off-by: Jackie Jone <jackie.jone@alliedtelesis.co.nz> >>>> --- >>>> drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c >>>> index 03a4da6a1447..7fbfcf01fbf9 100644 >>>> --- a/drivers/net/ethernet/intel/igb/igb_main.c >>>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c >>>> @@ -8977,6 +8977,10 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd) >>>> return -EIO; >>>> break; >>>> case SIOCSMIIREG: >>>> + if (igb_write_phy_reg(&adapter->hw, data->reg_num & 0x1F, >>>> + data->val_in)) >>>> + return -EIO; >>>> + break; >>> A handful of drivers seem to expose this. What are the consequences of >>> exposing this ioctl? What can user space do with it? >>> >>> It looks like a few drivers also check something like CAP_NET_ADMIN to >>> avoid allowing write access to all users. Is that enforced somewhere else? >> CAP_NET_ADMIN is enforced via dev_ioctl() so it should already be >> restricted to users with that capability. > Ok good. That at least limits this so that random users can't cause any > side effects. > > I'm not super familiar with what can be affected by writing the MII > registers. I'm also not sure what the community thinks of exposing such > access directly. > > From the description this is intended to use for debugging and testing > purposes? The immediate need is to provide access to some test mode registers that make the PHY output specific test patterns that can be observed with an oscilloscope. Our hardware colleagues use these to validate new hardware designs. On other products we have been using those "handful of drivers" that already support this, this is the first design we're we've needed it with igb. There is of course the alternative of exposing those test modes some other way but then we need to start enumerating what PHYs support which test modes. Some of these are defined in 802.3 but there are plenty of vendor extensions. One benefit I see in this is that does allow userland access to an MII device. I've used it to debug non-PHY devices like the mv88e6xxx L2 switch which has a management interface over MDIO. There's an in-kernel driver for this now so that specific usage isn't required but I bring it up as an example of a device that speaks MDIO but isn't a PHY. Whether this is a real advantage or not might depend on how you feel about userland drivers.
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On > Behalf Of Jacob Keller > Sent: Wednesday, June 5, 2024 11:17 PM > To: Chris Packham <Chris.Packham@alliedtelesis.co.nz>; Jackie Jone > <Jackie.Jone@alliedtelesis.co.nz>; davem@davemloft.net > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Nguyen, > Anthony L <anthony.l.nguyen@intel.com>; intel-wired- > lan@lists.osuosl.org; kuba@kernel.org > Subject: Re: [Intel-wired-lan] [PATCH] igb: Add MII write support > > > > On 6/5/2024 2:10 PM, Chris Packham wrote: > > > > On 6/06/24 08:51, Jacob Keller wrote: > >> > >> On 6/3/2024 8:10 PM, jackie.jone@alliedtelesis.co.nz wrote: > >>> From: Jackie Jone <jackie.jone@alliedtelesis.co.nz> > >>> > >>> To facilitate running PHY parametric tests, add support for the > >>> SIOCSMIIREG ioctl. This allows a userspace application to write > to > >>> the PHY registers to enable the test modes. > >>> > >>> Signed-off-by: Jackie Jone <jackie.jone@alliedtelesis.co.nz> > >>> --- > >>> drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++ > >>> 1 file changed, 4 insertions(+) > >>> > >>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c > >>> b/drivers/net/ethernet/intel/igb/igb_main.c > >>> index 03a4da6a1447..7fbfcf01fbf9 100644 > >>> --- a/drivers/net/ethernet/intel/igb/igb_main.c > >>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c > >>> @@ -8977,6 +8977,10 @@ static int igb_mii_ioctl(struct > net_device *netdev, struct ifreq *ifr, int cmd) > >>> return -EIO; > >>> break; > >>> case SIOCSMIIREG: > >>> + if (igb_write_phy_reg(&adapter->hw, data->reg_num & > 0x1F, > >>> + data->val_in)) > >>> + return -EIO; > >>> + break; > >> A handful of drivers seem to expose this. What are the > consequences > >> of exposing this ioctl? What can user space do with it? > >> > >> It looks like a few drivers also check something like > CAP_NET_ADMIN > >> to avoid allowing write access to all users. Is that enforced > somewhere else? > > > > CAP_NET_ADMIN is enforced via dev_ioctl() so it should already be > > restricted to users with that capability. > > Ok good. That at least limits this so that random users can't cause > any side effects. > I'm pretty sure from experience that even root applications will start cause nvmupdate issues. > I'm not super familiar with what can be affected by writing the MII > registers. I'm also not sure what the community thinks of exposing > such access directly. > > From the description this is intended to use for debugging and > testing purposes?
On Wed, Jun 05, 2024 at 01:51:24PM -0700, Jacob Keller wrote: > > > On 6/3/2024 8:10 PM, jackie.jone@alliedtelesis.co.nz wrote: > > From: Jackie Jone <jackie.jone@alliedtelesis.co.nz> > > > > To facilitate running PHY parametric tests, add support for the SIOCSMIIREG > > ioctl. This allows a userspace application to write to the PHY registers > > to enable the test modes. > > > > Signed-off-by: Jackie Jone <jackie.jone@alliedtelesis.co.nz> > > --- > > drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c > > index 03a4da6a1447..7fbfcf01fbf9 100644 > > --- a/drivers/net/ethernet/intel/igb/igb_main.c > > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > > @@ -8977,6 +8977,10 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd) > > return -EIO; > > break; > > case SIOCSMIIREG: > > + if (igb_write_phy_reg(&adapter->hw, data->reg_num & 0x1F, > > + data->val_in)) > > + return -EIO; > > + break; > > A handful of drivers seem to expose this. What are the consequences of > exposing this ioctl? What can user space do with it? User space can break the PHY configuration, cause the link to fail, all behind the MAC drivers back. The generic version of this call tries to see what registers are being written, and update state: https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy.c#L325 But you can still break it. > It looks like a few drivers also check something like CAP_NET_ADMIN to > avoid allowing write access to all users. Is that enforced somewhere else? Only root is allowed to use it. So it is a classic 'You have the option to shoot yourself in the foot'. For the use case being talked about here, there has been a few emails one the list about implementing the IEEE 802.3 test modes. But nobody has actually got around to doing it. Not that it would help in this case, Intel don't use the Linux PHY drivers, which is where i would expect to see such code implemented first. Maybe if the "Great Intel Ethernet driver refactoring" makes progress, it could swap to using the Linux PHY drivers. Andrew
On 6/6/2024 8:41 AM, Andrew Lunn wrote: > On Wed, Jun 05, 2024 at 01:51:24PM -0700, Jacob Keller wrote: >> >> >> On 6/3/2024 8:10 PM, jackie.jone@alliedtelesis.co.nz wrote: >>> From: Jackie Jone <jackie.jone@alliedtelesis.co.nz> >>> >>> To facilitate running PHY parametric tests, add support for the SIOCSMIIREG >>> ioctl. This allows a userspace application to write to the PHY registers >>> to enable the test modes. >>> >>> Signed-off-by: Jackie Jone <jackie.jone@alliedtelesis.co.nz> >>> --- >>> drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c >>> index 03a4da6a1447..7fbfcf01fbf9 100644 >>> --- a/drivers/net/ethernet/intel/igb/igb_main.c >>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c >>> @@ -8977,6 +8977,10 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd) >>> return -EIO; >>> break; >>> case SIOCSMIIREG: >>> + if (igb_write_phy_reg(&adapter->hw, data->reg_num & 0x1F, >>> + data->val_in)) >>> + return -EIO; >>> + break; >> >> A handful of drivers seem to expose this. What are the consequences of >> exposing this ioctl? What can user space do with it? > > User space can break the PHY configuration, cause the link to fail, > all behind the MAC drivers back. The generic version of this call > tries to see what registers are being written, and update state: > > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy.c#L325 > > But you can still break it. > Yea, its extremely easy to break things if you don't know what you're doing here. So its more a question of "are we ok exposing yet another way root can brick things?" >> It looks like a few drivers also check something like CAP_NET_ADMIN to >> avoid allowing write access to all users. Is that enforced somewhere else? > > Only root is allowed to use it. So it is a classic 'You have the > option to shoot yourself in the foot'. > I don't have an objection to enabling this myself, but I do want to be cognizant of the way it is viewed in the wider community. > For the use case being talked about here, there has been a few emails > one the list about implementing the IEEE 802.3 test modes. But nobody > has actually got around to doing it. Not that it would help in this > case, Intel don't use the Linux PHY drivers, which is where i would > expect to see such code implemented first. Maybe if the "Great Intel > Ethernet driver refactoring" makes progress, it could swap to using > the Linux PHY drivers. > > Andrew I remember this coming up several times in the past. I've always tried to push for it, but so far unsuccessfully.
> Yea, its extremely easy to break things if you don't know what you're > doing here. So its more a question of "are we ok exposing yet another > way root can brick things?" Many MAC drivers allow it, and we have not had complaints. It is not really something i'm a fan of, it in theory allows user space drivers for PHYs, but it is full of race conditions so in practice unlikely to work reliably. If you are worried about it causing additional support issues because it gets abused, you could make it taint the kernel. That makes it clear all bets are off if used. For the use case presented here, a tainted kernel does not matter, it for lab testing, not production. Andrew
On 6/6/2024 11:31 AM, Andrew Lunn wrote: >> Yea, its extremely easy to break things if you don't know what you're >> doing here. So its more a question of "are we ok exposing yet another >> way root can brick things?" > > Many MAC drivers allow it, and we have not had complaints. It is not > really something i'm a fan of, it in theory allows user space drivers > for PHYs, but it is full of race conditions so in practice unlikely to > work reliably. > > If you are worried about it causing additional support issues because > it gets abused, you could make it taint the kernel. That makes it > clear all bets are off if used. For the use case presented here, a > tainted kernel does not matter, it for lab testing, not production. > > Andrew > This might be a good compromise. I don't feel too strong either way regarding tainting the kernel. Adding support here I think will overall be more helpful than harmful. I like the tainting idea as a way to help reduce support burden and flag that something like this was done. That being said, I don't think I personally have a problem with the patch as-is given its intended use case so either way from me: Acked-by: Jacob Keller <jacob.e.keller@intel.com>
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of jackie.jone@alliedtelesis.co.nz > Sent: Tuesday, June 4, 2024 8:40 AM > To: davem@davemloft.net > Cc: jackie.jone@alliedtelesis.co.nz; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; chris.packham@alliedtelesis.co.nz; intel-wired-lan@lists.osuosl.org; kuba@kernel.org > Subject: [Intel-wired-lan] [PATCH] igb: Add MII write support > > From: Jackie Jone <jackie.jone@alliedtelesis.co.nz> > > To facilitate running PHY parametric tests, add support for the SIOCSMIIREG ioctl. This allows a userspace application to write to the PHY registers to enable the test modes. > > Signed-off-by: Jackie Jone <jackie.jone@alliedtelesis.co.nz> > --- > drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++ > 1 file changed, 4 insertions(+) > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c index 03a4da6a1447..7fbfcf01fbf9 100644 --- a/drivers/net/ethernet/intel/igb/igb_main.c +++ b/drivers/net/ethernet/intel/igb/igb_main.c @@ -8977,6 +8977,10 @@ static int igb_mii_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd) return -EIO; break; case SIOCSMIIREG: + if (igb_write_phy_reg(&adapter->hw, data->reg_num & 0x1F, + data->val_in)) + return -EIO; + break; default: return -EOPNOTSUPP; }