[PATCH net-next] net: phy: avoid NPE if read_page/write_page callbacks are not available

[PATCH net-next] net: phy: avoid NPE if read_page/write_page callbacks are not available

[Heiner Kallweit]

Currently there's a bug in the module subsystem [0] preventing load of
the PHY driver module on certain systems (as one symptom).
This results in a NPE on such systems for the following reason:
Instead of the correct PHY driver the genphy driver is loaded that
doesn't implement the read_page/write_page callbacks. Every call to
phy_read_paged() et al will result in a NPE therefore.

In parallel to fixing the root cause we should make sure that this one
and maybe similar issues in other subsystems don't result in a NPE
in phylib. So let's check for the callbacks before using them and warn
once if they are not available.

[0] https://marc.info/?t=157072642100001&r=1&w=2

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy-core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 9412669b5..0ae1722ba 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -689,11 +689,17 @@ EXPORT_SYMBOL_GPL(phy_modify_mmd);
 
 static int __phy_read_page(struct phy_device *phydev)
 {
+	if (WARN_ONCE(!phydev->drv->read_page, "read_page callback not available, PHY driver not loaded?\n"))
+		return -EOPNOTSUPP;
+
 	return phydev->drv->read_page(phydev);
 }
 
 static int __phy_write_page(struct phy_device *phydev, int page)
 {
+	if (WARN_ONCE(!phydev->drv->write_page, "write_page callback not available, PHY driver not loaded?\n"))
+		return -EOPNOTSUPP;
+
 	return phydev->drv->write_page(phydev, page);
 }
 
-- 
2.23.0

Re: [PATCH net-next] net: phy: avoid NPE if read_page/write_page callbacks are not available

[Florian Fainelli]

On 10/16/19 12:53 PM, Heiner Kallweit wrote:
> Currently there's a bug in the module subsystem [0] preventing load of
> the PHY driver module on certain systems (as one symptom).
> This results in a NPE on such systems for the following reason:
> Instead of the correct PHY driver the genphy driver is loaded that
> doesn't implement the read_page/write_page callbacks. Every call to
> phy_read_paged() et al will result in a NPE therefore.
> 
> In parallel to fixing the root cause we should make sure that this one
> and maybe similar issues in other subsystems don't result in a NPE
> in phylib. So let's check for the callbacks before using them and warn
> once if they are not available.

Everywhere else in the PHY library we tend to do:

if (!phydev->drv)
	return -EIO;

maybe not the best choice for an error code, but we should be consistent.

Is the issue really that we do have a driver we are bound to, but
somehow we cannot resolve the read_page/write_page callbacks to a valid
function pointer?

> 
> [0] https://marc.info/?t=157072642100001&r=1&w=2
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/phy/phy-core.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
> index 9412669b5..0ae1722ba 100644
> --- a/drivers/net/phy/phy-core.c
> +++ b/drivers/net/phy/phy-core.c
> @@ -689,11 +689,17 @@ EXPORT_SYMBOL_GPL(phy_modify_mmd);
>  
>  static int __phy_read_page(struct phy_device *phydev)
>  {
> +	if (WARN_ONCE(!phydev->drv->read_page, "read_page callback not available, PHY driver not loaded?\n"))
> +		return -EOPNOTSUPP;
> +
>  	return phydev->drv->read_page(phydev);
>  }
>  
>  static int __phy_write_page(struct phy_device *phydev, int page)
>  {
> +	if (WARN_ONCE(!phydev->drv->write_page, "write_page callback not available, PHY driver not loaded?\n"))
> +		return -EOPNOTSUPP;
> +
>  	return phydev->drv->write_page(phydev, page);
>  }
>  
> 


-- 
Florian

Re: [PATCH net-next] net: phy: avoid NPE if read_page/write_page callbacks are not available

[Heiner Kallweit]

On 16.10.2019 22:20, Florian Fainelli wrote:
> On 10/16/19 12:53 PM, Heiner Kallweit wrote:
>> Currently there's a bug in the module subsystem [0] preventing load of
>> the PHY driver module on certain systems (as one symptom).
>> This results in a NPE on such systems for the following reason:
>> Instead of the correct PHY driver the genphy driver is loaded that
>> doesn't implement the read_page/write_page callbacks. Every call to
>> phy_read_paged() et al will result in a NPE therefore.
>>
>> In parallel to fixing the root cause we should make sure that this one
>> and maybe similar issues in other subsystems don't result in a NPE
>> in phylib. So let's check for the callbacks before using them and warn
>> once if they are not available.
> 
> Everywhere else in the PHY library we tend to do:
> 
> if (!phydev->drv)
> 	return -EIO;
> 
A driver is bound, but genphy instead of the dedicated one.
I think this check won't catch the error.

> maybe not the best choice for an error code, but we should be consistent.
> 
> Is the issue really that we do have a driver we are bound to, but
> somehow we cannot resolve the read_page/write_page callbacks to a valid
> function pointer?
> 
The issue is that loading the dedicated PHY driver module fails, therefore
the (built-in) genphy driver is bound. Code in the r8169 driver expects
that the dedicated driver for the internal PHY is bound and uses
phy_read_page() et al what fails miserably with the genphy driver.

>>
>> [0] https://marc.info/?t=157072642100001&r=1&w=2
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/phy/phy-core.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
>> index 9412669b5..0ae1722ba 100644
>> --- a/drivers/net/phy/phy-core.c
>> +++ b/drivers/net/phy/phy-core.c
>> @@ -689,11 +689,17 @@ EXPORT_SYMBOL_GPL(phy_modify_mmd);
>>  
>>  static int __phy_read_page(struct phy_device *phydev)
>>  {
>> +	if (WARN_ONCE(!phydev->drv->read_page, "read_page callback not available, PHY driver not loaded?\n"))
>> +		return -EOPNOTSUPP;
>> +
>>  	return phydev->drv->read_page(phydev);
>>  }
>>  
>>  static int __phy_write_page(struct phy_device *phydev, int page)
>>  {
>> +	if (WARN_ONCE(!phydev->drv->write_page, "write_page callback not available, PHY driver not loaded?\n"))
>> +		return -EOPNOTSUPP;
>> +
>>  	return phydev->drv->write_page(phydev, page);
>>  }
>>  
>>
> 
> 

Re: [PATCH net-next] net: phy: avoid NPE if read_page/write_page callbacks are not available

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Wed, 16 Oct 2019 21:53:31 +0200

> Currently there's a bug in the module subsystem [0] preventing load of
> the PHY driver module on certain systems (as one symptom).
> This results in a NPE on such systems for the following reason:
> Instead of the correct PHY driver the genphy driver is loaded that
> doesn't implement the read_page/write_page callbacks. Every call to
> phy_read_paged() et al will result in a NPE therefore.
> 
> In parallel to fixing the root cause we should make sure that this one
> and maybe similar issues in other subsystems don't result in a NPE
> in phylib. So let's check for the callbacks before using them and warn
> once if they are not available.
> 
> [0] https://marc.info/?t=157072642100001&r=1&w=2
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

I read the discussion over a few times and this looks good to me for now,
so applied.

Thanks.