Received-SPF: pass (google.com: domain of linux-kernel+bounces-204745-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <c12ffb8e-0606-442b-810a-69bf65624bf9@intel.com>
Date: Thu, 6 Jun 2024 09:56:08 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] igb: Add MII write support
To: Andrew Lunn <andrew@lunn.ch>
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>
References: <20240604031020.2313175-1-jackie.jone@alliedtelesis.co.nz>
 <ad56235d-d267-4477-9c35-210309286ff4@intel.com>
 <1dbb8291-9004-4ec2-a01b-9dd5b2a8be39@lunn.ch>
Content-Language: en-US
From: Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <1dbb8291-9004-4ec2-a01b-9dd5b2a8be39@lunn.ch>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RVhaOERYTmZ5bDFxV0hGajZDa1lxdkNsWXhZM21TK3dScWRSMzh6a1RlTjJR?=
 =?utf-8?B?L3Y1S1ZrR0cxVERvbk1vTVdpY0loaTdqRTdGSXllaXVCSmEwbHNFaVRjeFJJ?=
 =?utf-8?B?VmlkVXhIeHVzRUhCbGJYcmcrNjNwVFYvU2g4citLanpoUkoxc3FCOHZLdnBa?=
 =?utf-8?B?VlRvbVdETTFLWE0wV2t0cmJQT0k5YzR0Z3dISVA4N2tWSkFxeWdSS2xPNnhq?=
 =?utf-8?B?aHRpRDhQWEhRVFUyYkVRb1pYSVdLUVlLellzbE5zTXM0UGN3bXF1WHMxejJH?=
 =?utf-8?B?UTVSR1dIbmRoZERsTmZHM1RSUDVHUWlYQXpkVlJzV3pzK1dVeFd2bnFHTkNh?=
 =?utf-8?B?UkVwZndCSGVkVFlNaG5OOVJXOXFEOGlLNVB2NXlFb0d2TWFvR3ZvTWZDUlFI?=
 =?utf-8?B?ZmRkWTVqWDdXVlIwalczMk5SSkJ3T1NwRmVGL3JvWXloQXowemNQc3YvTEZR?=
 =?utf-8?B?a0owejM2RHBuODNDYjFuS01DYXpEZ3B0dDNWQlBHWnNaZW1NY01IeHNURTla?=
 =?utf-8?B?WVVRTzFjT2lNRFl0U0Jub1l0TG5aczVzT216Z3VrQlNEZXhsMU9mWXlRUFBD?=
 =?utf-8?B?YnUycVgyLzF5Mi9vZWt2QlJUNFp0STRnbkNsUVFFOXRLVkJpOXZ0TzIzV29k?=
 =?utf-8?B?bzI5SHpvNUtBbTFwcnBpMkZ1Y2Z5TitCdlhIcGR2b3RVeFRvYWc1ZXFZOWpN?=
 =?utf-8?B?cWc1L05Kb1c1MVFUK2dFTkZld3cyVzVzYmkyWW50aUh6c2lkcUdWU0NmNERR?=
 =?utf-8?B?ZFBabldIcitneW9RdEZ3T3dFdkppUnk1bjN2b3JrV29UTmwzNDF1dSs0KzVX?=
 =?utf-8?B?bnV6a2svT3Q1TnZtbW1kcmxKVlRYaGlMWnd1b1o4THplOHBCa1M0d2VoSXp1?=
 =?utf-8?B?WFVMQWZVemZGeSt5c0E1R2JhT2NPZEtoZHIwODZaaHBOVkRHT0ljSEJDRFdD?=
 =?utf-8?B?UXQ0TmQrREtKUXBMc2ZxVEEvaW1YYThWN01mN1oyYjUrd3hLaHhQYUh6VlMw?=
 =?utf-8?B?MDFHdlRXU3NVS0lnbzB2N0JtNzZZQVF2bnlPcnBOdTBoRUx3NCtNZkxlbHBt?=
 =?utf-8?B?L3FEOWZYczF0dURGeGlSVWFHRVUxenBQeU9TQWhEV3N4anA3WjdZVStmTFp5?=
 =?utf-8?B?NkJBUnVUSTdJUU80d0JnckFmVS9YSHRKZW5OUFhENm9HQ1FUNVBGaTlzUUNr?=
 =?utf-8?B?dFlqYnRIRVpHb3ptczFHY1FDc3g4ajF6WnZtdXRndjF1ZzMyRFRYSWhlRzNF?=
 =?utf-8?B?OC91YndZMEN5anZpR3V6a3RRSHVndklrQ25uOGFqZzlwM3YyZUh4N1F1ak10?=
 =?utf-8?B?QlJqRm9oMzMrSEc1Y1c5NDBpeGRiaXFRN0RUaWNCZ2UxMFRnRGJCR20zMXhM?=
 =?utf-8?B?YTlsNXBETzVwUjhEczcxY1JGR3h3TWZQcmdCZTZjODVxOGNGV09LaGorR1Zw?=
 =?utf-8?B?MFh5cTVxYVliUXAyd3hPZ29Nb3NqcmN2RFNQUDBZVHJtQTJ3eS9oMUYvVU5l?=
 =?utf-8?B?UExwbnNUNzhFM2luOE53dHNtRkZiQmZGK2xOWkw5blErWDFGR3VXeHpaVnFq?=
 =?utf-8?B?cU9SSWcrMElpV2NidmFvYkU2UEVyYks3Y2pxNlNJQVRVSDhINDBSdHVxcVJR?=
 =?utf-8?B?cjEzNzh6YXY1ajNHeFhITFBPUysrM2dlQjFaVFJiaHB6REc1V0Nsc1pFbW16?=
 =?utf-8?B?bGozN1VnbDJzMCs0VmpmQ2FxczA2RlRMWGJ4V2RBdzk1bnFTSU55S21zaWdX?=
 =?utf-8?B?ME1rZ0JkaVVrSWIwc20xRmdROTQ3bnYwQTVFQ3JIVXM2T1ZWVENSNkJ5c1Fh?=
 =?utf-8?B?b1BZTWNYR3B2bHBybW9hTGQ5N2hqNFFEdWdUT1V4Rnk4TE5wRlFsaSs1UUJj?=
 =?utf-8?B?UTdUQWFmditzTmxteGgwOHFjc2hUTFJTVVNrZ3R1NmMzd3Y1MTBxaGxQLyto?=
 =?utf-8?B?Qmc0UFdaWXFMSTRyejlqVmdGa1ZNdzV2QUhYQW1OeGd3TjQ3YmNPUnZYazho?=
 =?utf-8?B?TFpXRVoxQUhhaC8rdTF3ZW13OEw4UnB0YXZ1SFJjNmpzQ0VRbENTZExuUVVS?=
 =?utf-8?B?T2NlY3NpZHpSMFg1aDV2dVk2RDRYcURwRVpOc3lTQ2wzMnNlM3lsWlpSd2Fu?=
 =?utf-8?B?VE9PenJ3bDZIVFNmd0pFQ21ydEpmSXg2TUY2K2xiMUEwbUhqUjdNcitCWU9N?=
 =?utf-8?B?ZFE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2605a6bf-08a1-401f-9b8e-08dc86499109
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Jun 2024 16:56:10.4832
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: PjdJwcXc+j167o7Os+e4hN6VV7n4tRblL08h3LT4YGy9JvNwNvT/u5FRf+TqN/fDeqJfB7AY8k84FNd6DEd5/3udMJYatj83LGMrVZpMgd0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB8270
X-OriginatorOrg: intel.com


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.