diff mbox series

[net-next] genetlink: add explicit ordering break check for split ops

Message ID 20230720111354.562242-1-jiri@resnulli.us (mailing list archive)
State Accepted
Commit 5766946ea5117e4edeb78c80cac367fb06854cc1
Delegated to: Netdev Maintainers
Headers show
Series [net-next] genetlink: add explicit ordering break check for split ops | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1344 this patch: 1344
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 1365 this patch: 1365
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1367 this patch: 1367
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 13 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 July 20, 2023, 11:13 a.m. UTC 
From: Jiri Pirko <jiri@nvidia.com>

Currently, if cmd in the split ops array is of lower value than the
previous one, genl_validate_ops() continues to do the checks as if
the values are equal. This may result in non-obvious WARN_ON() hit in
these check.

Instead, check the incorrect ordering explicitly and put a WARN_ON()
in case it is broken.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/netlink/genetlink.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Jakub Kicinski July 21, 2023, 2:32 a.m. UTC | #1 
On Thu, 20 Jul 2023 13:13:54 +0200 Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Currently, if cmd in the split ops array is of lower value than the
> previous one, genl_validate_ops() continues to do the checks as if
> the values are equal. This may result in non-obvious WARN_ON() hit in
> these check.
> 
> Instead, check the incorrect ordering explicitly and put a WARN_ON()
> in case it is broken.
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>

Reviewed-by: Jakub Kicinski <kuba@kernel.org>
Jacob Keller July 21, 2023, 4:45 p.m. UTC | #2 
> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Thursday, July 20, 2023 4:14 AM
> To: netdev@vger.kernel.org
> Cc: kuba@kernel.org; pabeni@redhat.com; davem@davemloft.net;
> edumazet@google.com; Keller, Jacob E <jacob.e.keller@intel.com>
> Subject: [patch net-next] genetlink: add explicit ordering break check for split ops
> 
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Currently, if cmd in the split ops array is of lower value than the
> previous one, genl_validate_ops() continues to do the checks as if
> the values are equal. This may result in non-obvious WARN_ON() hit in
> these check.
> 
> Instead, check the incorrect ordering explicitly and put a WARN_ON()
> in case it is broken.
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>


Good fix.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> ---
>  net/netlink/genetlink.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index a157247a1e45..6bd2ce51271f 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -593,8 +593,12 @@ static int genl_validate_ops(const struct genl_family
> *family)
>  			return -EINVAL;
> 
>  		/* Check sort order */
> -		if (a->cmd < b->cmd)
> +		if (a->cmd < b->cmd) {
>  			continue;
> +		} else if (a->cmd > b->cmd) {
> +			WARN_ON(1);
> +			return -EINVAL;
> +		}
> 
>  		if (a->internal_flags != b->internal_flags ||
>  		    ((a->flags ^ b->flags) & ~(GENL_CMD_CAP_DO |
> --
> 2.41.0
patchwork-bot+netdevbpf@kernel.org July 22, 2023, 2:10 a.m. UTC | #3 
Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 20 Jul 2023 13:13:54 +0200 you wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Currently, if cmd in the split ops array is of lower value than the
> previous one, genl_validate_ops() continues to do the checks as if
> the values are equal. This may result in non-obvious WARN_ON() hit in
> these check.
> 
> [...]

Here is the summary with links:
  - [net-next] genetlink: add explicit ordering break check for split ops
    https://git.kernel.org/netdev/net-next/c/5766946ea511

You are awesome, thank you!
diff mbox series

Patch

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index a157247a1e45..6bd2ce51271f 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -593,8 +593,12 @@  static int genl_validate_ops(const struct genl_family *family)
 			return -EINVAL;
 
 		/* Check sort order */
-		if (a->cmd < b->cmd)
+		if (a->cmd < b->cmd) {
 			continue;
+		} else if (a->cmd > b->cmd) {
+			WARN_ON(1);
+			return -EINVAL;
+		}
 
 		if (a->internal_flags != b->internal_flags ||
 		    ((a->flags ^ b->flags) & ~(GENL_CMD_CAP_DO |