diff mbox series

[net-next] devlink: don't allow to change net namespace for FW_ACTIVATE reload action

Message ID 20230210115827.3099567-1-jiri@resnulli.us (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next] devlink: don't allow to change net namespace for FW_ACTIVATE reload action | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2 this patch: 2
netdev/cc_maintainers fail 1 blamed authors not CCed: moshe@mellanox.com; 1 maintainers not CCed: moshe@mellanox.com
netdev/build_clang success Errors and warnings before: 1 this patch: 1
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 2 this patch: 2
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 11 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jiri Pirko Feb. 10, 2023, 11:58 a.m. UTC 
From: Jiri Pirko <jiri@nvidia.com>

The change on network namespace only makes sense during re-init reload
action. For FW activation it is not applicable. So check if user passed
an ATTR indicating network namespace change request and forbid it.

Fixes: ccdf07219da6 ("devlink: Add reload action option to devlink reload command")
Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
Sending to net-next as this is not actually fixing any real bug,
it just adds a forgotten check.
---
 net/devlink/dev.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Simon Horman Feb. 10, 2023, 1:15 p.m. UTC | #1 
On Fri, Feb 10, 2023 at 12:58:27PM +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> The change on network namespace only makes sense during re-init reload
> action. For FW activation it is not applicable. So check if user passed
> an ATTR indicating network namespace change request and forbid it.
> 
> Fixes: ccdf07219da6 ("devlink: Add reload action option to devlink reload command")
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---
> Sending to net-next as this is not actually fixing any real bug,
> it just adds a forgotten check.
> ---
>  net/devlink/dev.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/devlink/dev.c b/net/devlink/dev.c
> index 78d824eda5ec..a6a2bcded723 100644
> --- a/net/devlink/dev.c
> +++ b/net/devlink/dev.c
> @@ -474,6 +474,11 @@ int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
>  	if (info->attrs[DEVLINK_ATTR_NETNS_PID] ||
>  	    info->attrs[DEVLINK_ATTR_NETNS_FD] ||
>  	    info->attrs[DEVLINK_ATTR_NETNS_ID]) {
> +		if (action != DEVLINK_RELOAD_ACTION_DRIVER_REINIT) {
> +			NL_SET_ERR_MSG_MOD(info->extack,
> +					   "Changing namespace is only supported for reinit action");
> +			return -EOPNOTSUPP;
> +		}

Is this also applicable in the case where the requested ns (dest_net)
is the same as the current ns, which I think means that the ns
is not changed?

>  		dest_net = devlink_netns_get(skb, info);
>  		if (IS_ERR(dest_net))
>  			return PTR_ERR(dest_net);
> -- 
> 2.39.0
>
Jacob Keller Feb. 10, 2023, 7:43 p.m. UTC | #2 
On 2/10/2023 5:15 AM, Simon Horman wrote:
> On Fri, Feb 10, 2023 at 12:58:27PM +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>>
>> The change on network namespace only makes sense during re-init reload
>> action. For FW activation it is not applicable. So check if user passed
>> an ATTR indicating network namespace change request and forbid it.
>>
>> Fixes: ccdf07219da6 ("devlink: Add reload action option to devlink reload command")
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>> Sending to net-next as this is not actually fixing any real bug,
>> it just adds a forgotten check.
>> ---
>>  net/devlink/dev.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/net/devlink/dev.c b/net/devlink/dev.c
>> index 78d824eda5ec..a6a2bcded723 100644
>> --- a/net/devlink/dev.c
>> +++ b/net/devlink/dev.c
>> @@ -474,6 +474,11 @@ int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
>>  	if (info->attrs[DEVLINK_ATTR_NETNS_PID] ||
>>  	    info->attrs[DEVLINK_ATTR_NETNS_FD] ||
>>  	    info->attrs[DEVLINK_ATTR_NETNS_ID]) {
>> +		if (action != DEVLINK_RELOAD_ACTION_DRIVER_REINIT) {
>> +			NL_SET_ERR_MSG_MOD(info->extack,
>> +					   "Changing namespace is only supported for reinit action");
>> +			return -EOPNOTSUPP;
>> +		}
> 
> Is this also applicable in the case where the requested ns (dest_net)
> is the same as the current ns, which I think means that the ns
> is not changed?
> 

In that case wouldn't userspace simply not add the attribute though?

Thanks,
Jake
Simon Horman Feb. 11, 2023, 1:23 p.m. UTC | #3 
On Fri, Feb 10, 2023 at 11:43:04AM -0800, Jacob Keller wrote:
> 
> 
> On 2/10/2023 5:15 AM, Simon Horman wrote:
> > On Fri, Feb 10, 2023 at 12:58:27PM +0100, Jiri Pirko wrote:
> >> From: Jiri Pirko <jiri@nvidia.com>
> >>
> >> The change on network namespace only makes sense during re-init reload
> >> action. For FW activation it is not applicable. So check if user passed
> >> an ATTR indicating network namespace change request and forbid it.
> >>
> >> Fixes: ccdf07219da6 ("devlink: Add reload action option to devlink reload command")
> >> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> >> ---
> >> Sending to net-next as this is not actually fixing any real bug,
> >> it just adds a forgotten check.
> >> ---
> >>  net/devlink/dev.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/net/devlink/dev.c b/net/devlink/dev.c
> >> index 78d824eda5ec..a6a2bcded723 100644
> >> --- a/net/devlink/dev.c
> >> +++ b/net/devlink/dev.c
> >> @@ -474,6 +474,11 @@ int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
> >>  	if (info->attrs[DEVLINK_ATTR_NETNS_PID] ||
> >>  	    info->attrs[DEVLINK_ATTR_NETNS_FD] ||
> >>  	    info->attrs[DEVLINK_ATTR_NETNS_ID]) {
> >> +		if (action != DEVLINK_RELOAD_ACTION_DRIVER_REINIT) {
> >> +			NL_SET_ERR_MSG_MOD(info->extack,
> >> +					   "Changing namespace is only supported for reinit action");
> >> +			return -EOPNOTSUPP;
> >> +		}
> > 
> > Is this also applicable in the case where the requested ns (dest_net)
> > is the same as the current ns, which I think means that the ns
> > is not changed?
> > 
> 
> In that case wouldn't userspace simply not add the attribute though?

Yes, that may be the case.
But my question is about the correct behaviour if user space doesn't do that.
Jiri Pirko Feb. 13, 2023, 10:45 a.m. UTC | #4 
Sat, Feb 11, 2023 at 02:23:28PM CET, simon.horman@corigine.com wrote:
>On Fri, Feb 10, 2023 at 11:43:04AM -0800, Jacob Keller wrote:
>> 
>> 
>> On 2/10/2023 5:15 AM, Simon Horman wrote:
>> > On Fri, Feb 10, 2023 at 12:58:27PM +0100, Jiri Pirko wrote:
>> >> From: Jiri Pirko <jiri@nvidia.com>
>> >>
>> >> The change on network namespace only makes sense during re-init reload
>> >> action. For FW activation it is not applicable. So check if user passed
>> >> an ATTR indicating network namespace change request and forbid it.
>> >>
>> >> Fixes: ccdf07219da6 ("devlink: Add reload action option to devlink reload command")
>> >> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> >> ---
>> >> Sending to net-next as this is not actually fixing any real bug,
>> >> it just adds a forgotten check.
>> >> ---
>> >>  net/devlink/dev.c | 5 +++++
>> >>  1 file changed, 5 insertions(+)
>> >>
>> >> diff --git a/net/devlink/dev.c b/net/devlink/dev.c
>> >> index 78d824eda5ec..a6a2bcded723 100644
>> >> --- a/net/devlink/dev.c
>> >> +++ b/net/devlink/dev.c
>> >> @@ -474,6 +474,11 @@ int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
>> >>  	if (info->attrs[DEVLINK_ATTR_NETNS_PID] ||
>> >>  	    info->attrs[DEVLINK_ATTR_NETNS_FD] ||
>> >>  	    info->attrs[DEVLINK_ATTR_NETNS_ID]) {
>> >> +		if (action != DEVLINK_RELOAD_ACTION_DRIVER_REINIT) {
>> >> +			NL_SET_ERR_MSG_MOD(info->extack,
>> >> +					   "Changing namespace is only supported for reinit action");
>> >> +			return -EOPNOTSUPP;
>> >> +		}
>> > 
>> > Is this also applicable in the case where the requested ns (dest_net)
>> > is the same as the current ns, which I think means that the ns
>> > is not changed?
>> > 
>> 
>> In that case wouldn't userspace simply not add the attribute though?
>
>Yes, that may be the case.
>But my question is about the correct behaviour if user space doesn't do that.

Okay. Will send v2.
diff mbox series

Patch

diff --git a/net/devlink/dev.c b/net/devlink/dev.c
index 78d824eda5ec..a6a2bcded723 100644
--- a/net/devlink/dev.c
+++ b/net/devlink/dev.c
@@ -474,6 +474,11 @@  int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
 	if (info->attrs[DEVLINK_ATTR_NETNS_PID] ||
 	    info->attrs[DEVLINK_ATTR_NETNS_FD] ||
 	    info->attrs[DEVLINK_ATTR_NETNS_ID]) {
+		if (action != DEVLINK_RELOAD_ACTION_DRIVER_REINIT) {
+			NL_SET_ERR_MSG_MOD(info->extack,
+					   "Changing namespace is only supported for reinit action");
+			return -EOPNOTSUPP;
+		}
 		dest_net = devlink_netns_get(skb, info);
 		if (IS_ERR(dest_net))
 			return PTR_ERR(dest_net);