Received-SPF: pass (google.com: domain of linux-kernel+bounces-204592-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 6 Jun 2024 17:41:20 +0200
From: Andrew Lunn <andrew@lunn.ch>
To: Jacob Keller <jacob.e.keller@intel.com>
Cc: jackie.jone@alliedtelesis.co.nz, davem@davemloft.net,
	jesse.brandeburg@intel.com, anthony.l.nguyen@intel.com,
	kuba@kernel.org, intel-wired-lan@lists.osuosl.org,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	chris.packham@alliedtelesis.co.nz
Subject: Re: [PATCH] igb: Add MII write support
Message-ID: <1dbb8291-9004-4ec2-a01b-9dd5b2a8be39@lunn.ch>
References: <20240604031020.2313175-1-jackie.jone@alliedtelesis.co.nz>
 <ad56235d-d267-4477-9c35-210309286ff4@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ad56235d-d267-4477-9c35-210309286ff4@intel.com>

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