| Message ID | 5301F32C.4040704@huawei.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 02/17/2014 12:31 PM, Wang Weidong wrote: > some drivers maybe not implement the ethtool_ops with only > set NULL. So when call the ethtool cmds will lead to a > 'NULL pointer dereference'. > > So add a checking in dev_ethtool. > > Signed-off-by: Wang Weidong <wangweidong1@huawei.com> > --- > net/core/ethtool.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > index 30071de..f418dcb 100644 > --- a/net/core/ethtool.c > +++ b/net/core/ethtool.c > @@ -1499,6 +1499,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) > if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) > return -EPERM; > } > + You have a trailing whitespace/tab in the line above. Please use checkpatch for detecting such things. Can you be more specific with "some drivers"? Any driver that is in the mainline tree? > + if (!dev->ethtool_ops) > + return -EOPNOTSUPP; > > if (dev->ethtool_ops->begin) { > rc = dev->ethtool_ops->begin(dev); > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014/2/18 1:09, Daniel Borkmann wrote: > On 02/17/2014 12:31 PM, Wang Weidong wrote: >> some drivers maybe not implement the ethtool_ops with only >> set NULL. So when call the ethtool cmds will lead to a >> 'NULL pointer dereference'. >> >> So add a checking in dev_ethtool. >> >> Signed-off-by: Wang Weidong <wangweidong1@huawei.com> >> --- >> net/core/ethtool.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/net/core/ethtool.c b/net/core/ethtool.c >> index 30071de..f418dcb 100644 >> --- a/net/core/ethtool.c >> +++ b/net/core/ethtool.c >> @@ -1499,6 +1499,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) >> if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) >> return -EPERM; >> } >> + > > You have a trailing whitespace/tab in the line above. Please > use checkpatch for detecting such things. > Sorry for that. I will fix it soon. > Can you be more specific with "some drivers"? Any driver that > is in the mainline tree? > No. My team implements a driver without considering the ethtool_ops. So I got the problem. >> + if (!dev->ethtool_ops) >> + return -EOPNOTSUPP; >> >> if (dev->ethtool_ops->begin) { >> rc = dev->ethtool_ops->begin(dev); >> > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/18/2014 02:20 AM, Wang Weidong wrote: > On 2014/2/18 1:09, Daniel Borkmann wrote: >> On 02/17/2014 12:31 PM, Wang Weidong wrote: >>> some drivers maybe not implement the ethtool_ops with only >>> set NULL. So when call the ethtool cmds will lead to a >>> 'NULL pointer dereference'. >>> >>> So add a checking in dev_ethtool. >>> >>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com> >>> --- >>> net/core/ethtool.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c >>> index 30071de..f418dcb 100644 >>> --- a/net/core/ethtool.c >>> +++ b/net/core/ethtool.c >>> @@ -1499,6 +1499,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) >>> if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) >>> return -EPERM; >>> } >>> + >> >> You have a trailing whitespace/tab in the line above. Please >> use checkpatch for detecting such things. >> > Sorry for that. I will fix it soon. > >> Can you be more specific with "some drivers"? Any driver that >> is in the mainline tree? >> > No. My team implements a driver without considering the ethtool_ops. > So I got the problem. If it's code that is out of the mainline tree, then why should the kernel support that? Submit your driver to the tree first, and then such a change could be considered. Or, even better, let them implement ethtool ops/stubs. >>> + if (!dev->ethtool_ops) >>> + return -EOPNOTSUPP; >>> >>> if (dev->ethtool_ops->begin) { >>> rc = dev->ethtool_ops->begin(dev); >>> >> >> > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014/2/18 17:22, Daniel Borkmann wrote: > On 02/18/2014 02:20 AM, Wang Weidong wrote: >> On 2014/2/18 1:09, Daniel Borkmann wrote: >>> On 02/17/2014 12:31 PM, Wang Weidong wrote: >>>> some drivers maybe not implement the ethtool_ops with only >>>> set NULL. So when call the ethtool cmds will lead to a >>>> 'NULL pointer dereference'. >>>> >>>> So add a checking in dev_ethtool. >>>> >>>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com> >>>> --- >>>> net/core/ethtool.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c >>>> index 30071de..f418dcb 100644 >>>> --- a/net/core/ethtool.c >>>> +++ b/net/core/ethtool.c >>>> @@ -1499,6 +1499,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) >>>> if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) >>>> return -EPERM; >>>> } >>>> + >>> >>> You have a trailing whitespace/tab in the line above. Please >>> use checkpatch for detecting such things. >>> >> Sorry for that. I will fix it soon. >> >>> Can you be more specific with "some drivers"? Any driver that >>> is in the mainline tree? >>> >> No. My team implements a driver without considering the ethtool_ops. >> So I got the problem. > > If it's code that is out of the mainline tree, then why should the > kernel support that? Submit your driver to the tree first, and then > such a change could be considered. Or, even better, let them implement > ethtool ops/stubs. > Thanks for your reply. I will suggest them to implement ethtool ops. Regards Wang >>>> + if (!dev->ethtool_ops) >>>> + return -EOPNOTSUPP; >>>> >>>> if (dev->ethtool_ops->begin) { >>>> rc = dev->ethtool_ops->begin(dev); >>>> >>> >>> >> >> > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/ethtool.c b/net/core/ethtool.c index 30071de..f418dcb 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -1499,6 +1499,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) return -EPERM; } + + if (!dev->ethtool_ops) + return -EOPNOTSUPP; if (dev->ethtool_ops->begin) { rc = dev->ethtool_ops->begin(dev);
some drivers maybe not implement the ethtool_ops with only set NULL. So when call the ethtool cmds will lead to a 'NULL pointer dereference'. So add a checking in dev_ethtool. Signed-off-by: Wang Weidong <wangweidong1@huawei.com> --- net/core/ethtool.c | 3 +++ 1 file changed, 3 insertions(+)