| Message ID | 20190222220758.7117-7-jakub.kicinski@netronome.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | devlink: make ethtool compat reliable | expand |
Fri, Feb 22, 2019 at 11:07:57PM CET, jakub.kicinski@netronome.com wrote: >Commit 76726ccb7f46 ("devlink: add flash update command") and >commit 2d8dc5bbf4e7 ("devlink: Add support for reload") >access devlink ops without NULL-checking. Add the missing checks. >Note that all drivers currently implementing devlink pass non-NULL >ops, so this is not a problem. Wouldn't it be better to rather put WARN_ON&fail when driver calls devlink_alloc() with NULL ops and avoid these checks in the whole code? > >Reported-by: Florian Fainelli <f.fainelli@gmail.com> >Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> >--- > net/core/devlink.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/net/core/devlink.c b/net/core/devlink.c >index 78c6ea1870ca..38cb0239dede 100644 >--- a/net/core/devlink.c >+++ b/net/core/devlink.c >@@ -2651,7 +2651,7 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info) > struct devlink *devlink = info->user_ptr[0]; > int err; > >- if (!devlink->ops->reload) >+ if (!devlink->ops || !devlink->ops->reload) > return -EOPNOTSUPP; > > err = devlink_resources_validate(devlink, NULL, info); >@@ -2669,7 +2669,7 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb, > const char *file_name, *component; > struct nlattr *nla_component; > >- if (!devlink->ops->flash_update) >+ if (!devlink->ops || !devlink->ops->flash_update) > return -EOPNOTSUPP; > > if (!info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME]) >-- >2.19.2 >
On Sun, 24 Feb 2019 12:03:19 +0100, Jiri Pirko wrote: > Fri, Feb 22, 2019 at 11:07:57PM CET, jakub.kicinski@netronome.com wrote: >> Commit 76726ccb7f46 ("devlink: add flash update command") and >> commit 2d8dc5bbf4e7 ("devlink: Add support for reload") >> access devlink ops without NULL-checking. Add the missing checks. >> Note that all drivers currently implementing devlink pass non-NULL >> ops, so this is not a problem. > > Wouldn't it be better to rather put WARN_ON&fail when driver calls > devlink_alloc() with NULL ops and avoid these checks in the whole code? Sounds good! Should I remove the existing ones?
Mon, Feb 25, 2019 at 07:32:49PM CET, jakub.kicinski@netronome.com wrote: >On Sun, 24 Feb 2019 12:03:19 +0100, Jiri Pirko wrote: >> Fri, Feb 22, 2019 at 11:07:57PM CET, jakub.kicinski@netronome.com wrote: >>> Commit 76726ccb7f46 ("devlink: add flash update command") and >>> commit 2d8dc5bbf4e7 ("devlink: Add support for reload") >>> access devlink ops without NULL-checking. Add the missing checks. >>> Note that all drivers currently implementing devlink pass non-NULL >>> ops, so this is not a problem. >> >> Wouldn't it be better to rather put WARN_ON&fail when driver calls >> devlink_alloc() with NULL ops and avoid these checks in the whole code? > >Sounds good! Should I remove the existing ones? Yes please.
diff --git a/net/core/devlink.c b/net/core/devlink.c index 78c6ea1870ca..38cb0239dede 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -2651,7 +2651,7 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info) struct devlink *devlink = info->user_ptr[0]; int err; - if (!devlink->ops->reload) + if (!devlink->ops || !devlink->ops->reload) return -EOPNOTSUPP; err = devlink_resources_validate(devlink, NULL, info); @@ -2669,7 +2669,7 @@ static int devlink_nl_cmd_flash_update(struct sk_buff *skb, const char *file_name, *component; struct nlattr *nla_component; - if (!devlink->ops->flash_update) + if (!devlink->ops || !devlink->ops->flash_update) return -EOPNOTSUPP; if (!info->attrs[DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME])
Commit 76726ccb7f46 ("devlink: add flash update command") and commit 2d8dc5bbf4e7 ("devlink: Add support for reload") access devlink ops without NULL-checking. Add the missing checks. Note that all drivers currently implementing devlink pass non-NULL ops, so this is not a problem. Reported-by: Florian Fainelli <f.fainelli@gmail.com> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> --- net/core/devlink.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)