@@ -545,6 +545,8 @@ int __ethtool_get_link_ksettings(struct net_device *dev,{ASSERT_RTNL();+if(!dev||!dev->ethtool_ops)+return-EOPNOTSUPP;
I do not believe dev can possibly be NULL at this point.
if (!dev->ethtool_ops->get_link_ksettings)
return -EOPNOTSUPP;
I tried to find an appropriate Fixes: tag.
It seems this particular bug was added either by
Fixes: 9856909c2abb ("net: bonding: use __ethtool_get_ksettings")
or generically in :
Fixes: 3f1ac7a700d0 ("net: ethtool: add new ETHTOOL_xLINKSETTINGS API")
I do not believe dev can possibly be NULL at this point.
if (!dev->ethtool_ops->get_link_ksettings)
return -EOPNOTSUPP;
I tried to find an appropriate Fixes: tag.
It seems this particular bug was added either by
Fixes: 9856909c2abb ("net: bonding: use __ethtool_get_ksettings")
or generically in :
Fixes: 3f1ac7a700d0 ("net: ethtool: add new ETHTOOL_xLINKSETTINGS API")
In fact, "dev->ethtool_ops" is a null pointer in my environment.
I didn't get the case where "dev" is a null pointer.
Maybe "if (!dev->ethtool_ops)" is more accurate for this bug.
I found this bug in version 3.10, the function name was __ethtool_get_settings.
After 3f1ac7a700d0 ("net: ethtool: add new ETHTOOL_xLINKSETTINGS API"),
This function evolved into __ethtool_get_link_ksettings.
diff --git a/net/core/ethtool.c b/net/core/ethtool.c index
6288e69..9a50b64 100644
--- a/net/core/ethtool.c+++ b/net/core/ethtool.c
@@ -545,6 +545,8 @@ int __ethtool_get_link_ksettings(struct
net_device
*dev, {
ASSERT_RTNL();
+ if (!dev || !dev->ethtool_ops)
+ return -EOPNOTSUPP;
I do not believe dev can possibly be NULL at this point.
if (!dev->ethtool_ops->get_link_ksettings)
return -EOPNOTSUPP;
I tried to find an appropriate Fixes: tag.
It seems this particular bug was added either by
Fixes: 9856909c2abb ("net: bonding: use __ethtool_get_ksettings")
or generically in :
Fixes: 3f1ac7a700d0 ("net: ethtool: add new ETHTOOL_xLINKSETTINGS
API")
In fact, "dev->ethtool_ops" is a null pointer in my environment.
I didn't get the case where "dev" is a null pointer.
dev can't be a null pointer since bond driver guarantees that
and there is a check for the case where it could be null in
bond_slave_netdev_event.
You can drop the "!dev" check, since also it should be the caller
responsibility and we should avoid cluttering the net core code with
such redundant checks.
Maybe "if (!dev->ethtool_ops)" is more accurate for this bug.
Also i am not sure about this, could be a bug in the device driver your
enslaving.
alloc_netdev_mqs will assign &default_ethtool_ops to dev->ethtool_ops ,
if user provided setup callback didn't assign the driver specific
ethtool_ops.
so the device driver must be doing something wrong, overwriting defult
ethtool_ops with a NULL pointer maybe ? and why ?
I found this bug in version 3.10, the function name was
__ethtool_get_settings.
After 3f1ac7a700d0 ("net: ethtool: add new ETHTOOL_xLINKSETTINGS
API"),
This function evolved into __ethtool_get_link_ksettings.
From: Michal Kubecek <hidden> Date: 2019-08-29 06:01:42
On Tue, Aug 27, 2019 at 07:01:41PM +0000, Saeed Mahameed wrote:
On Mon, 2019-08-26 at 17:47 +0800, Dongxu Liu wrote:
quoted
Maybe "if (!dev->ethtool_ops)" is more accurate for this bug.
Also i am not sure about this, could be a bug in the device driver your
enslaving.
alloc_netdev_mqs will assign &default_ethtool_ops to dev->ethtool_ops ,
if user provided setup callback didn't assign the driver specific
ethtool_ops.
Dongxu said he encountered the null pointer dereference in a 3.10
kernel, not current mainline. But commit 2c60db037034 ("net: provide a
default dev->ethtool_ops") which introduced default_ethtool_ops came in
3.7-rc1 so 3.10 should have it already. There is indeed something wrong.
I don't think we should add either check unless we positively know that
dev->ethtool_ops can be null with current mainline kernel. And even
then, it would probably be more appropriate to fix the code which caused
it.
Michal