[PATCH] net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol

[PATCH] net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol

[Sebastian Hesselbarth]

phy_ethtool_get_wol is a helper to get current WOL settings from
a phy device. When using this helper on a PHY without .get_wol
callback, struct ethtool_wolinfo is never set-up correctly and
may contain misleading information about WOL status.

To fix this, always zero relevant fields of struct ethtool_wolinfo
regardless of .get_wol callback availability.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: netdev at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/net/phy/phy.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 19c9eca0ef26..62a7cd401e1c 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1092,6 +1092,7 @@ EXPORT_SYMBOL(phy_ethtool_set_wol);
 
 void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
 {
+	wol->supported = wol->wolopts = 0;
 	if (phydev->drv->get_wol)
 		phydev->drv->get_wol(phydev, wol);
 }
-- 
1.8.5.3

[PATCH] net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol

[Ben Hutchings]

On Mon, 2014-03-10 at 02:01 +0100, Sebastian Hesselbarth wrote:
> phy_ethtool_get_wol is a helper to get current WOL settings from
> a phy device. When using this helper on a PHY without .get_wol
> callback, struct ethtool_wolinfo is never set-up correctly and
> may contain misleading information about WOL status.
> 
> To fix this, always zero relevant fields of struct ethtool_wolinfo
> regardless of .get_wol callback availability.

I think it's the caller's responsibility to zero out struct
ethtool_wolinfo.  That is what ethtool_get_wol() does.

Maybe you could split ethtool_get_wol() like we did
ethtool_get_settings(), to support in-kernel invocation of ETHTOOL_GWOL?

Ben.

> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Cc: David Miller <davem@davemloft.net>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: netdev at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-kernel at vger.kernel.org
> ---
>  drivers/net/phy/phy.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 19c9eca0ef26..62a7cd401e1c 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1092,6 +1092,7 @@ EXPORT_SYMBOL(phy_ethtool_set_wol);
>  
>  void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
>  {
> +	wol->supported = wol->wolopts = 0;
>  	if (phydev->drv->get_wol)
>  		phydev->drv->get_wol(phydev, wol);
>  }

-- 
Ben Hutchings
Computers are not intelligent.	They only think they are.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 811 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140310/77d8477a/attachment.sig>

[PATCH] net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol

[Sebastian Hesselbarth]

On 03/10/2014 02:51 AM, Ben Hutchings wrote:
> On Mon, 2014-03-10 at 02:01 +0100, Sebastian Hesselbarth wrote:
>> phy_ethtool_get_wol is a helper to get current WOL settings from
>> a phy device. When using this helper on a PHY without .get_wol
>> callback, struct ethtool_wolinfo is never set-up correctly and
>> may contain misleading information about WOL status.
>>
>> To fix this, always zero relevant fields of struct ethtool_wolinfo
>> regardless of .get_wol callback availability.
>
> I think it's the caller's responsibility to zero out struct
> ethtool_wolinfo.  That is what ethtool_get_wol() does.

Actually, phy_ethtool_get_wol is the caller here. This belongs to
a set of helpers that deal with phy_device, not netdev.

> Maybe you could split ethtool_get_wol() like we did
> ethtool_get_settings(), to support in-kernel invocation of ETHTOOL_GWOL?

Looking at the other users of phy_ethtool_get_wol (mv643xx_eth and
cpsw), both drivers use this helper to determine what to pass back
on the corresponding ethtool_get_wol call.

BTW, both drivers above do zero ethtool_wolinfo before calling
phy_ethtool_get_wol. I can either zero it in phy_suspend too or we
deal with it properly in phy_ethtool_get_wol instead:

void phy_ethtool_get_wol(struct phy_device *phydev, struct 
ethtool_wolinfo *wol)
{
	memset(wol, 0, sizeof(*wol));

	if (phydev && phydev->drv->get_wol)
		phydev->drv->get_wol(phydev, wol);
}

That would also simplify above drivers down to e.g:

static void cpsw_get_wol(struct net_device *ndev, struct ethtool_wolinfo 
*wol)
{
	struct cpsw_priv *priv = netdev_priv(ndev);
	int slave_no = cpsw_slave_index(priv);
	phy_ethtool_get_wol(priv->slaves[slave_no].phy, wol);
}

instead of:

static void cpsw_get_wol(struct net_device *ndev, struct ethtool_wolinfo 
*wol)
{
	struct cpsw_priv *priv = netdev_priv(ndev);
	int slave_no = cpsw_slave_index(priv);

	wol->supported = 0;
	wol->wolopts = 0;

	if (priv->slaves[slave_no].phy)
		phy_ethtool_get_wol(priv->slaves[slave_no].phy, wol);
}

>> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> ---
>> Cc: David Miller <davem@davemloft.net>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: netdev at vger.kernel.org
>> Cc: linux-arm-kernel at lists.infradead.org
>> Cc: linux-kernel at vger.kernel.org
>> ---
>>   drivers/net/phy/phy.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index 19c9eca0ef26..62a7cd401e1c 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -1092,6 +1092,7 @@ EXPORT_SYMBOL(phy_ethtool_set_wol);
>>
>>   void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
>>   {
>> +	wol->supported = wol->wolopts = 0;
>>   	if (phydev->drv->get_wol)
>>   		phydev->drv->get_wol(phydev, wol);
>>   }
>

[PATCH] net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol

[David Miller]

From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Date: Mon, 10 Mar 2014 10:49:53 +0000

> void phy_ethtool_get_wol(struct phy_device *phydev, struct
> ethtool_wolinfo *wol)
> {
> 	memset(wol, 0, sizeof(*wol));
> 
> 	if (phydev && phydev->drv->get_wol)
> 		phydev->drv->get_wol(phydev, wol);
> }
> 
> That would also simplify above drivers down to e.g:
> 
> static void cpsw_get_wol(struct net_device *ndev, struct
> ethtool_wolinfo *wol)
> {
> 	struct cpsw_priv *priv = netdev_priv(ndev);
> 	int slave_no = cpsw_slave_index(priv);
> 	phy_ethtool_get_wol(priv->slaves[slave_no].phy, wol);
> }
> 
> instead of:
> 
> static void cpsw_get_wol(struct net_device *ndev, struct
> ethtool_wolinfo *wol)
> {
> 	struct cpsw_priv *priv = netdev_priv(ndev);
> 	int slave_no = cpsw_slave_index(priv);
> 
> 	wol->supported = 0;
> 	wol->wolopts = 0;
> 
> 	if (priv->slaves[slave_no].phy)
> 		phy_ethtool_get_wol(priv->slaves[slave_no].phy, wol);
> }

Agreed, since phy_ethtool_get_wol() is the common routine used by the drivers,
we should make it clear the structure.

[PATCH] net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol

[Ben Hutchings]

On Mon, 2014-03-10 at 16:23 -0400, David Miller wrote:
> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Date: Mon, 10 Mar 2014 10:49:53 +0000
> 
> > void phy_ethtool_get_wol(struct phy_device *phydev, struct
> > ethtool_wolinfo *wol)
> > {
> >       memset(wol, 0, sizeof(*wol));
> > 
> >       if (phydev && phydev->drv->get_wol)
> >               phydev->drv->get_wol(phydev, wol);
> > }
> > 
> > That would also simplify above drivers down to e.g:
> > 
> > static void cpsw_get_wol(struct net_device *ndev, struct
> > ethtool_wolinfo *wol)
> > {
> >       struct cpsw_priv *priv = netdev_priv(ndev);
> >       int slave_no = cpsw_slave_index(priv);
> >       phy_ethtool_get_wol(priv->slaves[slave_no].phy, wol);
> > }
> > 
> > instead of:
> > 
> > static void cpsw_get_wol(struct net_device *ndev, struct
> > ethtool_wolinfo *wol)
> > {
> >       struct cpsw_priv *priv = netdev_priv(ndev);
> >       int slave_no = cpsw_slave_index(priv);
> > 
> >       wol->supported = 0;
> >       wol->wolopts = 0;
> > 
> >       if (priv->slaves[slave_no].phy)
> >               phy_ethtool_get_wol(priv->slaves[slave_no].phy, wol);
> > }
> 
> Agreed, since phy_ethtool_get_wol() is the common routine used by the drivers,
> we should make it clear the structure.

No, ethtool_get_wol() is the common routine used by *all* the drivers
and it initialises the struct properly.

Ben.

-- 
Ben Hutchings
Computers are not intelligent.	They only think they are.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 811 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140310/023296ad/attachment.sig>

[PATCH] net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol

[Ben Hutchings]

On Mon, 2014-03-10 at 10:49 +0000, Sebastian Hesselbarth wrote:
> On 03/10/2014 02:51 AM, Ben Hutchings wrote:
> > On Mon, 2014-03-10 at 02:01 +0100, Sebastian Hesselbarth wrote:
> >> phy_ethtool_get_wol is a helper to get current WOL settings from
> >> a phy device. When using this helper on a PHY without .get_wol
> >> callback, struct ethtool_wolinfo is never set-up correctly and
> >> may contain misleading information about WOL status.
> >>
> >> To fix this, always zero relevant fields of struct ethtool_wolinfo
> >> regardless of .get_wol callback availability.
> >
> > I think it's the caller's responsibility to zero out struct
> > ethtool_wolinfo.  That is what ethtool_get_wol() does.
> 
> Actually, phy_ethtool_get_wol is the caller here. This belongs to
> a set of helpers that deal with phy_device, not netdev.

Right, but ethtool_get_wol() is further up the stack and is responsible
for initialising the struct to defaults.

> > Maybe you could split ethtool_get_wol() like we did
> > ethtool_get_settings(), to support in-kernel invocation of ETHTOOL_GWOL?
> 
> Looking at the other users of phy_ethtool_get_wol (mv643xx_eth and
> cpsw), both drivers use this helper to determine what to pass back
> on the corresponding ethtool_get_wol call.
> 
> BTW, both drivers above do zero ethtool_wolinfo before calling
> phy_ethtool_get_wol. I can either zero it in phy_suspend too or we
> deal with it properly in phy_ethtool_get_wol instead:
> 
> void phy_ethtool_get_wol(struct phy_device *phydev, struct 
> ethtool_wolinfo *wol)
> {
> 	memset(wol, 0, sizeof(*wol));
> 
> 	if (phydev && phydev->drv->get_wol)
> 		phydev->drv->get_wol(phydev, wol);
> }
[...]

This trashes wol->cmd.  Don't do that.

Ben.

-- 
Ben Hutchings
Computers are not intelligent.	They only think they are.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 811 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140310/59c0486b/attachment-0001.sig>

[PATCH] net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol

[Sebastian Hesselbarth]

On 03/11/2014 12:17 AM, Ben Hutchings wrote:
> On Mon, 2014-03-10 at 10:49 +0000, Sebastian Hesselbarth wrote:
>> On 03/10/2014 02:51 AM, Ben Hutchings wrote:
>>> On Mon, 2014-03-10 at 02:01 +0100, Sebastian Hesselbarth wrote:
>>>> phy_ethtool_get_wol is a helper to get current WOL settings from
>>>> a phy device. When using this helper on a PHY without .get_wol
>>>> callback, struct ethtool_wolinfo is never set-up correctly and
>>>> may contain misleading information about WOL status.
>>>>
>>>> To fix this, always zero relevant fields of struct ethtool_wolinfo
>>>> regardless of .get_wol callback availability.
>>>
>>> I think it's the caller's responsibility to zero out struct
>>> ethtool_wolinfo.  That is what ethtool_get_wol() does.
>>
>> Actually, phy_ethtool_get_wol is the caller here. This belongs to
>> a set of helpers that deal with phy_device, not netdev.
>
> Right, but ethtool_get_wol() is further up the stack and is responsible
> for initialising the struct to defaults.

Ok, currently we have 3 users of phy_ethtool_get_wol():
- mv643xx_eth and cpsw use that in their .get_wol callback that is
   called on ethtool_get_wol().
- phy_suspend to determine if it is safe to suspend a PHY

With phy_suspend, I could use a kernel-compatible __ethtool_get_wol()
but that requires to have an .attached_dev as __ethtool_get_wol()
takes netdev. This would limit phy_suspend to attached PHYs while
even non-attached PHYs should be suspended.

OTOH, if we don't want phy_ethtool_get_wol to clear out ethtool_wol and
no dependency on .attached_dev, phy_suspend is the only place to
properly initialize ethtool_wol on this level.

>>> Maybe you could split ethtool_get_wol() like we did
>>> ethtool_get_settings(), to support in-kernel invocation of ETHTOOL_GWOL?
>>
>> Looking at the other users of phy_ethtool_get_wol (mv643xx_eth and
>> cpsw), both drivers use this helper to determine what to pass back
>> on the corresponding ethtool_get_wol call.
>>
>> BTW, both drivers above do zero ethtool_wolinfo before calling
>> phy_ethtool_get_wol. I can either zero it in phy_suspend too or we
>> deal with it properly in phy_ethtool_get_wol instead:
>>
>> void phy_ethtool_get_wol(struct phy_device *phydev, struct
>> ethtool_wolinfo *wol)
>> {
>> 	memset(wol, 0, sizeof(*wol));
>>
>> 	if (phydev && phydev->drv->get_wol)
>> 		phydev->drv->get_wol(phydev, wol);
>> }
> [...]
>
> This trashes wol->cmd.  Don't do that.

You are right on this one, I'll missed it and will fix it up.

Sebastian

[PATCH v2 0/3] net: phy: fix uninitalized WOL struct in phy_ethtool_get_wol

[Sebastian Hesselbarth]

David,

this is a small patch set based on a single patch sent earlier [1] to
fix uninitalized struct ethtool_wolinfo in phy_ethtool_get_wol.
Compared to the single patch, this clears the whole struct
ethtool_wolinfo passed to phy_ethtool_get_wol and adds a check for
non-NULL struct phy_device.

I also added two cleanup patches for current users of
phy_ethtool_get_wol. Those two patches are also based on v3.14-rc1 but
aren't really part of the fix. They can wait for v3.15 and I'll rebase
on request.

[1] https://lkml.org/lkml/2014/3/9/169

Sebastian Hesselbarth (3):
  net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol
  net: mv643xx_eth: simplify phy_ethtool_get_wol call
  net: cpsw: simplify phy_ethtool_get_wol call

 drivers/net/ethernet/marvell/mv643xx_eth.c | 5 +----
 drivers/net/ethernet/ti/cpsw.c             | 7 +------
 drivers/net/phy/phy.c                      | 4 +++-
 3 files changed, 5 insertions(+), 11 deletions(-)

---
Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: netdev at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
-- 
1.8.5.3

[PATCH v2 1/3] net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol

[Sebastian Hesselbarth]

phy_ethtool_get_wol is a helper to get current WOL settings from
a phy device. When using this helper on a PHY without .get_wol
callback, struct ethtool_wolinfo is never set-up correctly and
may contain misleading information about WOL status.

To fix this, always zero struct ethtool_wolinfo regardless of
.get_wol callback availability.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Changelog:
v1->v2:
- clear whole struct ethtool_wolinfo
- check for non-NULL phy_device

Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: netdev at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/net/phy/phy.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 19c9eca0ef26..dae1fa8aa23d 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1092,7 +1092,9 @@ EXPORT_SYMBOL(phy_ethtool_set_wol);
 
 void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
 {
-	if (phydev->drv->get_wol)
+	memset(wol, 0, sizeof(*wol));
+
+	if (phydev && phydev->drv->get_wol)
 		phydev->drv->get_wol(phydev, wol);
 }
 EXPORT_SYMBOL(phy_ethtool_get_wol);
-- 
1.8.5.3

[PATCH v3] net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol

[Sebastian Hesselbarth]

phy_ethtool_get_wol is a helper to get current WOL settings from
a phy device. When using this helper on a PHY without .get_wol
callback, struct ethtool_wolinfo is never set-up correctly and
may contain misleading information about WOL status.

To fix this, always zero relevant fields of struct ethtool_wolinfo
regardless of .get_wol callback availability.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changelog:
v1->v2:
- clear whole struct ethtool_wolinfo
- check for non-NULL phy_device
v2->v3:
- only clear ->supported and ->wolopts (Suggested by Ben Hutchings)

Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: netdev at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/net/phy/phy.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 19c9eca0ef26..94234a91a50f 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1092,7 +1092,9 @@ EXPORT_SYMBOL(phy_ethtool_set_wol);
 
 void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
 {
-	if (phydev->drv->get_wol)
+	wol->supported = wol->wolopts = 0;
+
+	if (phydev && phydev->drv->get_wol)
 		phydev->drv->get_wol(phydev, wol);
 }
 EXPORT_SYMBOL(phy_ethtool_get_wol);
-- 
1.8.5.3

[PATCH v3] net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol

[Ben Hutchings]

On Wed, 2014-03-12 at 00:02 +0100, Sebastian Hesselbarth wrote:
> phy_ethtool_get_wol is a helper to get current WOL settings from
> a phy device. When using this helper on a PHY without .get_wol
> callback, struct ethtool_wolinfo is never set-up correctly and
> may contain misleading information about WOL status.
> 
> To fix this, always zero relevant fields of struct ethtool_wolinfo
> regardless of .get_wol callback availability.

Sorry, I still disagree with this.

You're trying to make phy_ethtool_get_wol() do two subtly different
things:
- Provide an implementation of ethtool_ops::get_wol, leaving the net
  driver only to look up phy_device
- Provide a standalone function for executing ETHTOOL_GWOL on a
  phy_device

You may notice that phy_suspend() already sets wol.cmd = ETHTOOL_GWOL.
So it seems to me like it's taking responsibility for initialising the
structure like ethtool_get_wol() does.  The bug is then that
phy_suspend() doesn't clear the rest of the structure.  That is not the
responsibility of phy_ethtool_get_wol().

Ben.

> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changelog:
> v1->v2:
> - clear whole struct ethtool_wolinfo
> - check for non-NULL phy_device
> v2->v3:
> - only clear ->supported and ->wolopts (Suggested by Ben Hutchings)
> 
> Cc: David Miller <davem@davemloft.net>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Ben Hutchings <ben@decadent.org.uk>
> Cc: netdev at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-kernel at vger.kernel.org
> ---
>  drivers/net/phy/phy.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 19c9eca0ef26..94234a91a50f 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1092,7 +1092,9 @@ EXPORT_SYMBOL(phy_ethtool_set_wol);
>  
>  void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
>  {
> -	if (phydev->drv->get_wol)
> +	wol->supported = wol->wolopts = 0;
> +
> +	if (phydev && phydev->drv->get_wol)
>  		phydev->drv->get_wol(phydev, wol);
>  }
>  EXPORT_SYMBOL(phy_ethtool_get_wol);

-- 
Ben Hutchings
Experience is directly proportional to the value of equipment destroyed.
                                                         - Carolyn Scheppner
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 811 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140313/2bfd7788/attachment.sig>

[PATCH v3] net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol

[Sebastian Hesselbarth]

On 03/13/2014 02:21 AM, Ben Hutchings wrote:
> On Wed, 2014-03-12 at 00:02 +0100, Sebastian Hesselbarth wrote:
>> phy_ethtool_get_wol is a helper to get current WOL settings from
>> a phy device. When using this helper on a PHY without .get_wol
>> callback, struct ethtool_wolinfo is never set-up correctly and
>> may contain misleading information about WOL status.
>>
>> To fix this, always zero relevant fields of struct ethtool_wolinfo
>> regardless of .get_wol callback availability.
>
> Sorry, I still disagree with this.

Which is really fine with me. Thanks for constantly commenting this.

> You're trying to make phy_ethtool_get_wol() do two subtly different
> things:
> - Provide an implementation of ethtool_ops::get_wol, leaving the net
>    driver only to look up phy_device
> - Provide a standalone function for executing ETHTOOL_GWOL on a
>    phy_device
>
> You may notice that phy_suspend() already sets wol.cmd = ETHTOOL_GWOL.

Yeah, which was added by me because I misinterpreted phy_ethtool_get_wol
depending on it. It doesn't because we just "reuse" struct
ethtool_wolinfo.

> So it seems to me like it's taking responsibility for initialising the
> structure like ethtool_get_wol() does.  The bug is then that
> phy_suspend() doesn't clear the rest of the structure.  That is not the
> responsibility of phy_ethtool_get_wol().

I agree that public ethtool_get_wol should clear the struct, but I am
not so happy to have an in-kernel API depend on the caller to setup the
struct. In any way, if there are strong reasons not to clear struct
ethtool_wolinfo again in phy_ethtool_get_wol, I can properly clear it
in phy_suspend instead. I don't have a strong opinion about it.

Sebastian

>> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>> Changelog:
>> v1->v2:
>> - clear whole struct ethtool_wolinfo
>> - check for non-NULL phy_device
>> v2->v3:
>> - only clear ->supported and ->wolopts (Suggested by Ben Hutchings)
>>
>> Cc: David Miller <davem@davemloft.net>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: Ben Hutchings <ben@decadent.org.uk>
>> Cc: netdev at vger.kernel.org
>> Cc: linux-arm-kernel at lists.infradead.org
>> Cc: linux-kernel at vger.kernel.org
>> ---
>>   drivers/net/phy/phy.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index 19c9eca0ef26..94234a91a50f 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -1092,7 +1092,9 @@ EXPORT_SYMBOL(phy_ethtool_set_wol);
>>
>>   void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
>>   {
>> -	if (phydev->drv->get_wol)
>> +	wol->supported = wol->wolopts = 0;
>> +
>> +	if (phydev && phydev->drv->get_wol)
>>   		phydev->drv->get_wol(phydev, wol);
>>   }
>>   EXPORT_SYMBOL(phy_ethtool_get_wol);
>

[PATCH v3] net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol

[David Miller]

From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Date: Wed, 12 Mar 2014 00:02:55 +0100

> phy_ethtool_get_wol is a helper to get current WOL settings from
> a phy device. When using this helper on a PHY without .get_wol
> callback, struct ethtool_wolinfo is never set-up correctly and
> may contain misleading information about WOL status.
> 
> To fix this, always zero relevant fields of struct ethtool_wolinfo
> regardless of .get_wol callback availability.
> 
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

I'm starting to see this situation more clearly now, especially with
Ben's most recent commentary.

The basic notion is that one must do ethtool ops are designed such that
the top-level execution context in net/core/ethtool.c takes care of
initializing the structure.

In this case, we're referring specifically to ethtool_get_wol(), which
runs any time ETHTOOL_GWOL is requested.

Therefore no ethtool_ops->get_wol() implementation should duplicate
this work, that goes for all of such cases which invoke the function
we are talking about here, phy_ethtool_get_wol().

So the first change is definitely to remove:

	wol->supported = 0;
	wol->wolopts = 0;

from:

drivers/net/ethernet/marvell/mv643xx_eth.c:mv643xx_eth_get_wol()
drivers/net/ethernet/ti/cpsw.c:cpsw_get_wol()

Next, I think phy_suspend() must create the same environment the
call sites above guarentee for phy_ethtool_get_wol(), namely
by changing the declaration of 'wol' to:

	struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };

which will cause the compiler to clear out the rest of the structure
for us, as the same declaration does in ethtool_get_wol().

Finally, purge the spurious clears in phydev_ops->get_wol(), namely
in at803x_get_wol() and m88e1318_get_wol().

So, to reiterate, OPS never have to be mindful of initializing the
ethtool result with zeros.  However, anyone who calls into OPS
directly must provide said expected state.

Are we all on the same page now?

[PATCH v3] net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol

[Sebastian Hesselbarth]

On 03/13/2014 08:38 PM, David Miller wrote:
> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Date: Wed, 12 Mar 2014 00:02:55 +0100
>
>> phy_ethtool_get_wol is a helper to get current WOL settings from
>> a phy device. When using this helper on a PHY without .get_wol
>> callback, struct ethtool_wolinfo is never set-up correctly and
>> may contain misleading information about WOL status.
>>
>> To fix this, always zero relevant fields of struct ethtool_wolinfo
>> regardless of .get_wol callback availability.
>>
>> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>
> I'm starting to see this situation more clearly now, especially with
> Ben's most recent commentary.
>
> The basic notion is that one must do ethtool ops are designed such that
> the top-level execution context in net/core/ethtool.c takes care of
> initializing the structure.
>
> In this case, we're referring specifically to ethtool_get_wol(), which
> runs any time ETHTOOL_GWOL is requested.
>
> Therefore no ethtool_ops->get_wol() implementation should duplicate
> this work, that goes for all of such cases which invoke the function
> we are talking about here, phy_ethtool_get_wol().
>
> So the first change is definitely to remove:
>
> 	wol->supported = 0;
> 	wol->wolopts = 0;
>
> from:
>
> drivers/net/ethernet/marvell/mv643xx_eth.c:mv643xx_eth_get_wol()
> drivers/net/ethernet/ti/cpsw.c:cpsw_get_wol()
>
> Next, I think phy_suspend() must create the same environment the
> call sites above guarentee for phy_ethtool_get_wol(), namely
> by changing the declaration of 'wol' to:
>
> 	struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
>
> which will cause the compiler to clear out the rest of the structure
> for us, as the same declaration does in ethtool_get_wol().
>
> Finally, purge the spurious clears in phydev_ops->get_wol(), namely
> in at803x_get_wol() and m88e1318_get_wol().
>
> So, to reiterate, OPS never have to be mindful of initializing the
> ethtool result with zeros.  However, anyone who calls into OPS
> directly must provide said expected state.
>
> Are we all on the same page now?

Yes, we are. I'll send a fix for phy_suspend in a minute - still based
on v3.14-rc1 in case there will be another -rc.

If not, I'll rebase that fix on net-next together with patches for the
four offending drivers you named above.

Sebastian

[PATCH] net: phy: fix uninitalized ethtool_wolinfo in phy_suspend

[Sebastian Hesselbarth]

Callers of phy_ethtool_get_wol are supposed to provide a properly
cleared struct ethtool_wolinfo. Therefore, fix phy_suspend to clear
it before passing it to phy_ethtool_get_wol.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: netdev at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/net/phy/phy_device.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 4b03e63639b7..7512e28866f1 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -683,10 +683,9 @@ EXPORT_SYMBOL(phy_detach);
 int phy_suspend(struct phy_device *phydev)
 {
 	struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
-	struct ethtool_wolinfo wol;
+	struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
 
 	/* If the device has WOL enabled, we cannot suspend the PHY */
-	wol.cmd = ETHTOOL_GWOL;
 	phy_ethtool_get_wol(phydev, &wol);
 	if (wol.wolopts)
 		return -EBUSY;
-- 
1.8.5.3

[PATCH] net: phy: fix uninitalized ethtool_wolinfo in phy_suspend

[Ben Hutchings]

On Fri, 2014-03-14 at 10:07 +0100, Sebastian Hesselbarth wrote:
> Callers of phy_ethtool_get_wol are supposed to provide a properly
> cleared struct ethtool_wolinfo. Therefore, fix phy_suspend to clear
> it before passing it to phy_ethtool_get_wol.
> 
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

Reviewed-by: Ben Hutchings <ben@decadent.org.uk>

> ---
> Cc: David Miller <davem@davemloft.net>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Ben Hutchings <ben@decadent.org.uk>
> Cc: netdev at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-kernel at vger.kernel.org
> ---
>  drivers/net/phy/phy_device.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 4b03e63639b7..7512e28866f1 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -683,10 +683,9 @@ EXPORT_SYMBOL(phy_detach);
>  int phy_suspend(struct phy_device *phydev)
>  {
>  	struct phy_driver *phydrv = to_phy_driver(phydev->dev.driver);
> -	struct ethtool_wolinfo wol;
> +	struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
>  
>  	/* If the device has WOL enabled, we cannot suspend the PHY */
> -	wol.cmd = ETHTOOL_GWOL;
>  	phy_ethtool_get_wol(phydev, &wol);
>  	if (wol.wolopts)
>  		return -EBUSY;

-- 
Ben Hutchings
Experience is directly proportional to the value of equipment destroyed.
                                                         - Carolyn Scheppner
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 811 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140314/41d4409e/attachment.sig>

[PATCH] net: phy: fix uninitalized ethtool_wolinfo in phy_suspend

[David Miller]

From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Date: Fri, 14 Mar 2014 10:07:44 +0100

> Callers of phy_ethtool_get_wol are supposed to provide a properly
> cleared struct ethtool_wolinfo. Therefore, fix phy_suspend to clear
> it before passing it to phy_ethtool_get_wol.
> 
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

Applied, thanks.

[PATCH v3] net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol

[Sebastian Hesselbarth]

On 03/13/2014 08:38 PM, David Miller wrote:
> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Date: Wed, 12 Mar 2014 00:02:55 +0100
[...]
>> To fix this, always zero relevant fields of struct ethtool_wolinfo
>> regardless of .get_wol callback availability.
[...]
> I'm starting to see this situation more clearly now, especially with
> Ben's most recent commentary.
>
> The basic notion is that one must do ethtool ops are designed such that
> the top-level execution context in net/core/ethtool.c takes care of
> initializing the structure.
>
> In this case, we're referring specifically to ethtool_get_wol(), which
> runs any time ETHTOOL_GWOL is requested.
>
> Therefore no ethtool_ops->get_wol() implementation should duplicate
> this work, that goes for all of such cases which invoke the function
> we are talking about here, phy_ethtool_get_wol().
>
> So the first change is definitely to remove:
>
> 	wol->supported = 0;
> 	wol->wolopts = 0;
>
> from:
>
> drivers/net/ethernet/marvell/mv643xx_eth.c:mv643xx_eth_get_wol()
> drivers/net/ethernet/ti/cpsw.c:cpsw_get_wol()
>
[...]
>
> Finally, purge the spurious clears in phydev_ops->get_wol(), namely
> in at803x_get_wol() and m88e1318_get_wol().

David,

I was preparing cleanups for mv643xx_eth, cpsw, at803x, and mv88e1318.

Out of curiosity, I did a

git grep "wol->" drivers/net/ | grep "= 0" | wc -l
29

and found some other "spurious clears" ;)

I can go that road and remove/rework all those clears. Some are really
easy, some would require some more rework (e.g. e1000).

Of course, a lot of those drivers then will need a Tested-by, as I
don't have the HW available.

> So, to reiterate, OPS never have to be mindful of initializing the
> ethtool result with zeros.  However, anyone who calls into OPS
> directly must provide said expected state.

Sebastian

[PATCH v2 2/3] net: mv643xx_eth: simplify phy_ethtool_get_wol call

[Sebastian Hesselbarth]

With phy_ethtool_get_wol now clearing struct ethtool_wolinfo, we can
simplify routines calling it.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Changelog:
v1->v2:
- initial

Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: netdev at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/net/ethernet/marvell/mv643xx_eth.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index a2565ce22b7c..729464ecce2a 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -1336,10 +1336,7 @@ static void
 mv643xx_eth_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 {
 	struct mv643xx_eth_private *mp = netdev_priv(dev);
-	wol->supported = 0;
-	wol->wolopts = 0;
-	if (mp->phy)
-		phy_ethtool_get_wol(mp->phy, wol);
+	phy_ethtool_get_wol(mp->phy, wol);
 }
 
 static int
-- 
1.8.5.3

[PATCH v2 3/3] net: cpsw: simplify phy_ethtool_get_wol call

[Sebastian Hesselbarth]

With phy_ethtool_get_wol now clearing struct ethtool_wolinfo, we can
simplify routines calling it.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Changelog:
v1->v2:
- initial

Cc: David Miller <davem@davemloft.net>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: netdev at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/net/ethernet/ti/cpsw.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index bde63e3af96f..06950c75cb2f 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1726,12 +1726,7 @@ static void cpsw_get_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
 {
 	struct cpsw_priv *priv = netdev_priv(ndev);
 	int slave_no = cpsw_slave_index(priv);
-
-	wol->supported = 0;
-	wol->wolopts = 0;
-
-	if (priv->slaves[slave_no].phy)
-		phy_ethtool_get_wol(priv->slaves[slave_no].phy, wol);
+	phy_ethtool_get_wol(priv->slaves[slave_no].phy, wol);
 }
 
 static int cpsw_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
-- 
1.8.5.3

[PATCH v2 0/3] net: phy: fix uninitalized WOL struct in phy_ethtool_get_wol

[Florian Fainelli]

2014-03-10 13:50 GMT-07:00 Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com>:
> David,
>
> this is a small patch set based on a single patch sent earlier [1] to
> fix uninitalized struct ethtool_wolinfo in phy_ethtool_get_wol.
> Compared to the single patch, this clears the whole struct
> ethtool_wolinfo passed to phy_ethtool_get_wol and adds a check for
> non-NULL struct phy_device.
>
> I also added two cleanup patches for current users of
> phy_ethtool_get_wol. Those two patches are also based on v3.14-rc1 but
> aren't really part of the fix. They can wait for v3.15 and I'll rebase
> on request.

Looks good to me, thanks Sebastian:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

>
> [1] https://lkml.org/lkml/2014/3/9/169
>
> Sebastian Hesselbarth (3):
>   net: phy: fix uninitalized WOL parameters in phy_ethtool_get_wol
>   net: mv643xx_eth: simplify phy_ethtool_get_wol call
>   net: cpsw: simplify phy_ethtool_get_wol call
>
>  drivers/net/ethernet/marvell/mv643xx_eth.c | 5 +----
>  drivers/net/ethernet/ti/cpsw.c             | 7 +------
>  drivers/net/phy/phy.c                      | 4 +++-
>  3 files changed, 5 insertions(+), 11 deletions(-)
>
> ---
> Cc: David Miller <davem@davemloft.net>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: netdev at vger.kernel.org
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-kernel at vger.kernel.org
> --
> 1.8.5.3
>



-- 
Florian