[PATCH net-next] genetlink: fix policy dump for dumps

[PATCH net-next] genetlink: fix policy dump for dumps

[Jakub Kicinski]

Jonathan reports crashes when running net-next in Meta's fleet.
Stats collection uses ethtool -I which does a per-op policy dump
to check if stats are supported. We don't initialize the dumpit
information if doit succeeds due to evaluation short-circuiting.

The crash may look like this:

   BUG: kernel NULL pointer dereference, address: 0000000000000cc0
   RIP: 0010:netlink_policy_dump_add_policy+0x174/0x2a0
     ctrl_dumppolicy_start+0x19f/0x2f0
     genl_start+0xe7/0x140

Or we may trigger a warning:

   WARNING: CPU: 1 PID: 785 at net/netlink/policy.c:87 netlink_policy_dump_get_policy_idx+0x79/0x80
   RIP: 0010:netlink_policy_dump_get_policy_idx+0x79/0x80
     ctrl_dumppolicy_put_op+0x214/0x360

depending on what garbage we pick up from the stack.

Reported-by: Jonathan Lemon <bsd@meta.com>
Fixes: 26588edbef60 ("genetlink: support split policies in ctrl_dumppolicy_put_op()")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: jacob.e.keller@intel.com
---
 net/netlink/genetlink.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 9b7dfc45dd67..7b7bac9e7524 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1406,8 +1406,8 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
 		ctx->single_op = true;
 		ctx->op = nla_get_u32(tb[CTRL_ATTR_OP]);
 
-		if (genl_get_cmd(ctx->op, GENL_CMD_CAP_DO, rt, &doit) &&
-		    genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP, rt, &dump)) {
+		if (!!genl_get_cmd(ctx->op, GENL_CMD_CAP_DO, rt, &doit) +
+		    !!genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP, rt, &dump) < 1) {
 			NL_SET_BAD_ATTR(cb->extack, tb[CTRL_ATTR_OP]);
 			return -ENOENT;
 		}
@@ -1551,10 +1551,10 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
 		if (ctx->single_op) {
 			struct genl_split_ops doit, dumpit;
 
-			if (genl_get_cmd(ctx->op, GENL_CMD_CAP_DO,
-					 ctx->rt, &doit) &&
-			    genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP,
-					 ctx->rt, &dumpit)) {
+			if (!!genl_get_cmd(ctx->op, GENL_CMD_CAP_DO,
+					   ctx->rt, &doit) +
+			    !!genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP,
+					   ctx->rt, &dumpit) < 1) {
 				WARN_ON(1);
 				return -ENOENT;
 			}
-- 
2.38.1

RE: [PATCH net-next] genetlink: fix policy dump for dumps

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, November 8, 2022 12:41 PM
> To: davem@davemloft.net
> Cc: netdev@vger.kernel.org; edumazet@google.com; pabeni@redhat.com; Jakub
> Kicinski <kuba@kernel.org>; Jonathan Lemon <bsd@meta.com>; Keller, Jacob E
> <jacob.e.keller@intel.com>
> Subject: [PATCH net-next] genetlink: fix policy dump for dumps
> 
> Jonathan reports crashes when running net-next in Meta's fleet.
> Stats collection uses ethtool -I which does a per-op policy dump
> to check if stats are supported. We don't initialize the dumpit
> information if doit succeeds due to evaluation short-circuiting.
> 
> The crash may look like this:
> 
>    BUG: kernel NULL pointer dereference, address: 0000000000000cc0
>    RIP: 0010:netlink_policy_dump_add_policy+0x174/0x2a0
>      ctrl_dumppolicy_start+0x19f/0x2f0
>      genl_start+0xe7/0x140
> 
> Or we may trigger a warning:
> 
>    WARNING: CPU: 1 PID: 785 at net/netlink/policy.c:87
> netlink_policy_dump_get_policy_idx+0x79/0x80
>    RIP: 0010:netlink_policy_dump_get_policy_idx+0x79/0x80
>      ctrl_dumppolicy_put_op+0x214/0x360
> 
> depending on what garbage we pick up from the stack.
> 


Oops. Yay subtle bugs :D

> Reported-by: Jonathan Lemon <bsd@meta.com>
> Fixes: 26588edbef60 ("genetlink: support split policies in
> ctrl_dumppolicy_put_op()")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: jacob.e.keller@intel.com
> ---
>  net/netlink/genetlink.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 9b7dfc45dd67..7b7bac9e7524 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -1406,8 +1406,8 @@ static int ctrl_dumppolicy_start(struct netlink_callback
> *cb)
>  		ctx->single_op = true;
>  		ctx->op = nla_get_u32(tb[CTRL_ATTR_OP]);
> 
> -		if (genl_get_cmd(ctx->op, GENL_CMD_CAP_DO, rt, &doit) &&
> -		    genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP, rt, &dump)) {
> +		if (!!genl_get_cmd(ctx->op, GENL_CMD_CAP_DO, rt, &doit) +
> +		    !!genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP, rt, &dump) <
> 1) {


A little bit tricky code here, but it makes sense. We could rewrite this to be a bit more verbose like:

doit_err = genl_get_cmd(.. GENL_CMD_CAP_DO ..);
dumpit_err = genl_get_cmd(.. GENL_CMD_CAP_DUMPIT ..);
if (doit_err && dumpit_err) {
  ...
}

That might be a bit easier to read than the !! ( ) + ( ) < 1 notation.

Either way I think it looks correct at least.

Thanks,
Jake

>  			NL_SET_BAD_ATTR(cb->extack, tb[CTRL_ATTR_OP]);
>  			return -ENOENT;
>  		}
> @@ -1551,10 +1551,10 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct
> netlink_callback *cb)
>  		if (ctx->single_op) {
>  			struct genl_split_ops doit, dumpit;
> 
> -			if (genl_get_cmd(ctx->op, GENL_CMD_CAP_DO,
> -					 ctx->rt, &doit) &&
> -			    genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP,
> -					 ctx->rt, &dumpit)) {
> +			if (!!genl_get_cmd(ctx->op, GENL_CMD_CAP_DO,
> +					   ctx->rt, &doit) +
> +			    !!genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP,
> +					   ctx->rt, &dumpit) < 1) {
>  				WARN_ON(1);
>  				return -ENOENT;
>  			}
> --
> 2.38.1

Re: [PATCH net-next] genetlink: fix policy dump for dumps

[Jakub Kicinski]

On Tue, 8 Nov 2022 20:47:57 +0000 Keller, Jacob E wrote:
> A little bit tricky code here, but it makes sense. We could rewrite this to be a bit more verbose like:
> 
> doit_err = genl_get_cmd(.. GENL_CMD_CAP_DO ..);
> dumpit_err = genl_get_cmd(.. GENL_CMD_CAP_DUMPIT ..);
> if (doit_err && dumpit_err) {
>   ...
> }
> 
> That might be a bit easier to read than the !! ( ) + ( ) < 1 notation.

True, I should not give into the bit math temptations.

How about a helper:

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 9b7dfc45dd67..600993c80050 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -282,6 +282,7 @@ genl_cmd_full_to_split(struct genl_split_ops *op,
 	return 0;
 }
 
+/* Must make sure that op is initialized to 0 on failure */
 static int
 genl_get_cmd(u32 cmd, u8 flags, const struct genl_family *family,
 	     struct genl_split_ops *op)
@@ -302,6 +303,21 @@ genl_get_cmd(u32 cmd, u8 flags, const struct genl_family *family,
 	return err;
 }
 
+/* For policy dumping only, get ops of both do and dump.
+ * Fail if both are missing, genl_get_cmd() will zero-init in case of failure.
+ */
+static int
+genl_get_cmd_both(u32 cmd, const struct genl_family *family,
+		  struct genl_split_ops *doit, struct genl_split_ops *dumpit)
+{
+	int err1, err2;
+
+	err1 = genl_get_cmd(cmd, GENL_CMD_CAP_DO, family, doit);
+	err2 = genl_get_cmd(cmd, GENL_CMD_CAP_DUMP, family, dumpit);
+
+	return err1 && err2 ? -ENOENT : 0;
+}
+
 static bool
 genl_op_iter_init(const struct genl_family *family, struct genl_op_iter *iter)
 {
@@ -1406,10 +1422,10 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
 		ctx->single_op = true;
 		ctx->op = nla_get_u32(tb[CTRL_ATTR_OP]);
 
-		if (genl_get_cmd(ctx->op, GENL_CMD_CAP_DO, rt, &doit) &&
-		    genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP, rt, &dump)) {
+		err = genl_get_cmd_both(ctx->op, rt, &doit, &dump);
+		if (err) {
 			NL_SET_BAD_ATTR(cb->extack, tb[CTRL_ATTR_OP]);
-			return -ENOENT;
+			return err;
 		}
 
 		if (doit.policy) {
@@ -1551,13 +1567,9 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct netlink_callback *cb)
 		if (ctx->single_op) {
 			struct genl_split_ops doit, dumpit;
 
-			if (genl_get_cmd(ctx->op, GENL_CMD_CAP_DO,
-					 ctx->rt, &doit) &&
-			    genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP,
-					 ctx->rt, &dumpit)) {
-				WARN_ON(1);
+			if (WARN_ON(genl_get_cmd_both(ctx->op, ctx->rt,
+						      &doit, &dumpit)))
 				return -ENOENT;
-			}
 
 			if (ctrl_dumppolicy_put_op(skb, cb, &doit, &dumpit))
 				return skb->len;
-- 
2.38.1

RE: [PATCH net-next] genetlink: fix policy dump for dumps

[Keller, Jacob E]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, November 8, 2022 3:44 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: davem@davemloft.net; netdev@vger.kernel.org; edumazet@google.com;
> pabeni@redhat.com; Jonathan Lemon <bsd@meta.com>
> Subject: Re: [PATCH net-next] genetlink: fix policy dump for dumps
> 
> On Tue, 8 Nov 2022 20:47:57 +0000 Keller, Jacob E wrote:
> > A little bit tricky code here, but it makes sense. We could rewrite this to be a bit
> more verbose like:
> >
> > doit_err = genl_get_cmd(.. GENL_CMD_CAP_DO ..);
> > dumpit_err = genl_get_cmd(.. GENL_CMD_CAP_DUMPIT ..);
> > if (doit_err && dumpit_err) {
> >   ...
> > }
> >
> > That might be a bit easier to read than the !! ( ) + ( ) < 1 notation.
> 
> True, I should not give into the bit math temptations.
> 
> How about a helper:
> 

Much better, the little explanation about the zero init helps too.

Thanks,
Jake

> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 9b7dfc45dd67..600993c80050 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -282,6 +282,7 @@ genl_cmd_full_to_split(struct genl_split_ops *op,
>  	return 0;
>  }
> 
> +/* Must make sure that op is initialized to 0 on failure */
>  static int
>  genl_get_cmd(u32 cmd, u8 flags, const struct genl_family *family,
>  	     struct genl_split_ops *op)
> @@ -302,6 +303,21 @@ genl_get_cmd(u32 cmd, u8 flags, const struct
> genl_family *family,
>  	return err;
>  }
> 
> +/* For policy dumping only, get ops of both do and dump.
> + * Fail if both are missing, genl_get_cmd() will zero-init in case of failure.
> + */
> +static int
> +genl_get_cmd_both(u32 cmd, const struct genl_family *family,
> +		  struct genl_split_ops *doit, struct genl_split_ops *dumpit)
> +{
> +	int err1, err2;
> +
> +	err1 = genl_get_cmd(cmd, GENL_CMD_CAP_DO, family, doit);
> +	err2 = genl_get_cmd(cmd, GENL_CMD_CAP_DUMP, family, dumpit);
> +
> +	return err1 && err2 ? -ENOENT : 0;
> +}
> +
>  static bool
>  genl_op_iter_init(const struct genl_family *family, struct genl_op_iter *iter)
>  {
> @@ -1406,10 +1422,10 @@ static int ctrl_dumppolicy_start(struct
> netlink_callback *cb)
>  		ctx->single_op = true;
>  		ctx->op = nla_get_u32(tb[CTRL_ATTR_OP]);
> 
> -		if (genl_get_cmd(ctx->op, GENL_CMD_CAP_DO, rt, &doit) &&
> -		    genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP, rt, &dump)) {
> +		err = genl_get_cmd_both(ctx->op, rt, &doit, &dump);
> +		if (err) {
>  			NL_SET_BAD_ATTR(cb->extack, tb[CTRL_ATTR_OP]);
> -			return -ENOENT;
> +			return err;
>  		}
> 
>  		if (doit.policy) {
> @@ -1551,13 +1567,9 @@ static int ctrl_dumppolicy(struct sk_buff *skb, struct
> netlink_callback *cb)
>  		if (ctx->single_op) {
>  			struct genl_split_ops doit, dumpit;
> 
> -			if (genl_get_cmd(ctx->op, GENL_CMD_CAP_DO,
> -					 ctx->rt, &doit) &&
> -			    genl_get_cmd(ctx->op, GENL_CMD_CAP_DUMP,
> -					 ctx->rt, &dumpit)) {
> -				WARN_ON(1);
> +			if (WARN_ON(genl_get_cmd_both(ctx->op, ctx->rt,
> +						      &doit, &dumpit)))
>  				return -ENOENT;
> -			}
> 
>  			if (ctrl_dumppolicy_put_op(skb, cb, &doit, &dumpit))
>  				return skb->len;
> --
> 2.38.1

Re: [PATCH net-next] genetlink: fix policy dump for dumps

[Leon Romanovsky]

On Tue, Nov 08, 2022 at 12:40:41PM -0800, Jakub Kicinski wrote:
> Jonathan reports crashes when running net-next in Meta's fleet.

I experience these crashes too.

> Stats collection uses ethtool -I which does a per-op policy dump
> to check if stats are supported. We don't initialize the dumpit
> information if doit succeeds due to evaluation short-circuiting.
> 
> The crash may look like this:
> 
>    BUG: kernel NULL pointer dereference, address: 0000000000000cc0
>    RIP: 0010:netlink_policy_dump_add_policy+0x174/0x2a0
>      ctrl_dumppolicy_start+0x19f/0x2f0
>      genl_start+0xe7/0x140
> 
> Or we may trigger a warning:
> 
>    WARNING: CPU: 1 PID: 785 at net/netlink/policy.c:87 netlink_policy_dump_get_policy_idx+0x79/0x80
>    RIP: 0010:netlink_policy_dump_get_policy_idx+0x79/0x80
>      ctrl_dumppolicy_put_op+0x214/0x360
> 
> depending on what garbage we pick up from the stack.
> 
> Reported-by: Jonathan Lemon <bsd@meta.com>
> Fixes: 26588edbef60 ("genetlink: support split policies in ctrl_dumppolicy_put_op()")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: jacob.e.keller@intel.com
> ---
>  net/netlink/genetlink.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

I added your updated patch to my CI run. Unfortunately, the regression
system is overloaded due to nightly regression so won't be able to get
results in sensible time frame.

Thanks

Re: [PATCH net-next] genetlink: fix policy dump for dumps

[Leon Romanovsky]

On Wed, Nov 09, 2022 at 07:46:34PM +0200, Leon Romanovsky wrote:
> On Tue, Nov 08, 2022 at 12:40:41PM -0800, Jakub Kicinski wrote:
> > Jonathan reports crashes when running net-next in Meta's fleet.
> 
> I experience these crashes too.
> 
> > Stats collection uses ethtool -I which does a per-op policy dump
> > to check if stats are supported. We don't initialize the dumpit
> > information if doit succeeds due to evaluation short-circuiting.
> > 
> > The crash may look like this:
> > 
> >    BUG: kernel NULL pointer dereference, address: 0000000000000cc0
> >    RIP: 0010:netlink_policy_dump_add_policy+0x174/0x2a0
> >      ctrl_dumppolicy_start+0x19f/0x2f0
> >      genl_start+0xe7/0x140
> > 
> > Or we may trigger a warning:
> > 
> >    WARNING: CPU: 1 PID: 785 at net/netlink/policy.c:87 netlink_policy_dump_get_policy_idx+0x79/0x80
> >    RIP: 0010:netlink_policy_dump_get_policy_idx+0x79/0x80
> >      ctrl_dumppolicy_put_op+0x214/0x360
> > 
> > depending on what garbage we pick up from the stack.
> > 
> > Reported-by: Jonathan Lemon <bsd@meta.com>
> > Fixes: 26588edbef60 ("genetlink: support split policies in ctrl_dumppolicy_put_op()")
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > ---
> > CC: jacob.e.keller@intel.com
> > ---
> >  net/netlink/genetlink.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> I added your updated patch to my CI run. Unfortunately, the regression
> system is overloaded due to nightly regression so won't be able to get
> results in sensible time frame.
> 

Thanks,
Tested-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH net-next] genetlink: fix policy dump for dumps

[Jakub Kicinski]

On Wed, 9 Nov 2022 20:59:09 +0200 Leon Romanovsky wrote:
> > I added your updated patch to my CI run. Unfortunately, the regression
> > system is overloaded due to nightly regression so won't be able to get
> > results in sensible time frame.
> 
> Tested-by: Leon Romanovsky <leonro@nvidia.com>

To be clear - did you test as posted or v2? Or doesn't matter?
I'm wondering how applicable the tag is to v2.

Re: [PATCH net-next] genetlink: fix policy dump for dumps

[Saeed Mahameed]

On 09 Nov 12:11, Jakub Kicinski wrote:
>On Wed, 9 Nov 2022 20:59:09 +0200 Leon Romanovsky wrote:
>> > I added your updated patch to my CI run. Unfortunately, the regression
>> > system is overloaded due to nightly regression so won't be able to get
>> > results in sensible time frame.
>>
>> Tested-by: Leon Romanovsky <leonro@nvidia.com>
>
>To be clear - did you test as posted or v2? Or doesn't matter?
>I'm wondering how applicable the tag is to v2.

FWIW: I just tested v2 and it passed our CI.

Re: [PATCH net-next] genetlink: fix policy dump for dumps

[Leon Romanovsky]

On Wed, Nov 09, 2022 at 12:11:18PM -0800, Jakub Kicinski wrote:
> On Wed, 9 Nov 2022 20:59:09 +0200 Leon Romanovsky wrote:
> > > I added your updated patch to my CI run. Unfortunately, the regression
> > > system is overloaded due to nightly regression so won't be able to get
> > > results in sensible time frame.
> > 
> > Tested-by: Leon Romanovsky <leonro@nvidia.com>
> 
> To be clear - did you test as posted or v2? Or doesn't matter?
> I'm wondering how applicable the tag is to v2.

I tested this version https://lore.kernel.org/all/20221108154426.3a882067@kernel.org

Thanks