[PATCH net v1] net: dcb: choose correct policy to parse DCB_ATTR_BCN

[PATCH net v1] net: dcb: choose correct policy to parse DCB_ATTR_BCN

[Lin Ma]

The dcbnl_bcn_setcfg uses erroneous policy to parse tb[DCB_ATTR_BCN],
which is introduced in commit 859ee3c43812 ("DCB: Add support for DCB
BCN"). Please see the comment in below code

static int dcbnl_bcn_setcfg(...)
{
  ...
  ret = nla_parse_nested_deprecated(..., dcbnl_pfc_up_nest, .. )
  // !!! dcbnl_pfc_up_nest for attributes
  //  DCB_PFC_UP_ATTR_0 to DCB_PFC_UP_ATTR_ALL in enum dcbnl_pfc_up_attrs
  ...
  for (i = DCB_BCN_ATTR_RP_0; i <= DCB_BCN_ATTR_RP_7; i++) {
  // !!! DCB_BCN_ATTR_RP_0 to DCB_BCN_ATTR_RP_7 in enum dcbnl_bcn_attrs
    ...
    value_byte = nla_get_u8(data[i]);
    ...
  }
  ...
  for (i = DCB_BCN_ATTR_BCNA_0; i <= DCB_BCN_ATTR_RI; i++) {
  // !!! DCB_BCN_ATTR_BCNA_0 to DCB_BCN_ATTR_RI in enum dcbnl_bcn_attrs
  ...
    value_int = nla_get_u32(data[i]);
  ...
  }
  ...
}

That is, the nla_parse_nested_deprecated uses dcbnl_pfc_up_nest
attributes to parse nlattr defined in dcbnl_pfc_up_attrs. But the
following access code fetch each nlattr as dcbnl_bcn_attrs attributes.
By looking up the associated nla_policy for dcbnl_bcn_attrs. We can find
the beginning part of these two policies are "same".

static const struct nla_policy dcbnl_pfc_up_nest[...] = {
	[DCB_PFC_UP_ATTR_0]   = {.type = NLA_U8},
	[DCB_PFC_UP_ATTR_1]   = {.type = NLA_U8},
	[DCB_PFC_UP_ATTR_2]   = {.type = NLA_U8},
	[DCB_PFC_UP_ATTR_3]   = {.type = NLA_U8},
	[DCB_PFC_UP_ATTR_4]   = {.type = NLA_U8},
	[DCB_PFC_UP_ATTR_5]   = {.type = NLA_U8},
	[DCB_PFC_UP_ATTR_6]   = {.type = NLA_U8},
	[DCB_PFC_UP_ATTR_7]   = {.type = NLA_U8},
	[DCB_PFC_UP_ATTR_ALL] = {.type = NLA_FLAG},
};

static const struct nla_policy dcbnl_bcn_nest[...] = {
	[DCB_BCN_ATTR_RP_0]         = {.type = NLA_U8},
	[DCB_BCN_ATTR_RP_1]         = {.type = NLA_U8},
	[DCB_BCN_ATTR_RP_2]         = {.type = NLA_U8},
	[DCB_BCN_ATTR_RP_3]         = {.type = NLA_U8},
	[DCB_BCN_ATTR_RP_4]         = {.type = NLA_U8},
	[DCB_BCN_ATTR_RP_5]         = {.type = NLA_U8},
	[DCB_BCN_ATTR_RP_6]         = {.type = NLA_U8},
	[DCB_BCN_ATTR_RP_7]         = {.type = NLA_U8},
	[DCB_BCN_ATTR_RP_ALL]       = {.type = NLA_FLAG},
	// from here is somewhat different
	[DCB_BCN_ATTR_BCNA_0]       = {.type = NLA_U32},
        ...
        [DCB_BCN_ATTR_ALL]          = {.type = NLA_FLAG},
};

Therefore, the current code is buggy and this
nla_parse_nested_deprecated could overflow the dcbnl_pfc_up_nest and use
the adjacent nla_policy to parse attributes from DCB_BCN_ATTR_BCNA_0.

This patch use correct dcbnl_bcn_nest policy to parse the
tb[DCB_ATTR_BCN] nested TLV.

Fixes: 859ee3c43812 ("DCB: Add support for DCB BCN")
Signed-off-by: Lin Ma <linma@zju.edu.cn>
---
 net/dcb/dcbnl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c
index c0c438128575..2e6b8c8fd2de 100644
--- a/net/dcb/dcbnl.c
+++ b/net/dcb/dcbnl.c
@@ -980,7 +980,7 @@ static int dcbnl_bcn_setcfg(struct net_device *netdev, struct nlmsghdr *nlh,
 		return -EOPNOTSUPP;
 
 	ret = nla_parse_nested_deprecated(data, DCB_BCN_ATTR_MAX,
-					  tb[DCB_ATTR_BCN], dcbnl_pfc_up_nest,
+					  tb[DCB_ATTR_BCN], dcbnl_bcn_nest,
 					  NULL);
 	if (ret)
 		return ret;
-- 
2.17.1

Re: [PATCH net v1] net: dcb: choose correct policy to parse DCB_ATTR_BCN

[Simon Horman]

On Mon, Jul 31, 2023 at 12:52:16PM +0800, Lin Ma wrote:
> The dcbnl_bcn_setcfg uses erroneous policy to parse tb[DCB_ATTR_BCN],
> which is introduced in commit 859ee3c43812 ("DCB: Add support for DCB
> BCN"). Please see the comment in below code
> 
> static int dcbnl_bcn_setcfg(...)
> {
>   ...
>   ret = nla_parse_nested_deprecated(..., dcbnl_pfc_up_nest, .. )
>   // !!! dcbnl_pfc_up_nest for attributes
>   //  DCB_PFC_UP_ATTR_0 to DCB_PFC_UP_ATTR_ALL in enum dcbnl_pfc_up_attrs
>   ...
>   for (i = DCB_BCN_ATTR_RP_0; i <= DCB_BCN_ATTR_RP_7; i++) {
>   // !!! DCB_BCN_ATTR_RP_0 to DCB_BCN_ATTR_RP_7 in enum dcbnl_bcn_attrs
>     ...
>     value_byte = nla_get_u8(data[i]);
>     ...
>   }
>   ...
>   for (i = DCB_BCN_ATTR_BCNA_0; i <= DCB_BCN_ATTR_RI; i++) {
>   // !!! DCB_BCN_ATTR_BCNA_0 to DCB_BCN_ATTR_RI in enum dcbnl_bcn_attrs
>   ...
>     value_int = nla_get_u32(data[i]);
>   ...
>   }
>   ...
> }
> 
> That is, the nla_parse_nested_deprecated uses dcbnl_pfc_up_nest
> attributes to parse nlattr defined in dcbnl_pfc_up_attrs. But the
> following access code fetch each nlattr as dcbnl_bcn_attrs attributes.
> By looking up the associated nla_policy for dcbnl_bcn_attrs. We can find
> the beginning part of these two policies are "same".
> 
> static const struct nla_policy dcbnl_pfc_up_nest[...] = {
> 	[DCB_PFC_UP_ATTR_0]   = {.type = NLA_U8},
> 	[DCB_PFC_UP_ATTR_1]   = {.type = NLA_U8},
> 	[DCB_PFC_UP_ATTR_2]   = {.type = NLA_U8},
> 	[DCB_PFC_UP_ATTR_3]   = {.type = NLA_U8},
> 	[DCB_PFC_UP_ATTR_4]   = {.type = NLA_U8},
> 	[DCB_PFC_UP_ATTR_5]   = {.type = NLA_U8},
> 	[DCB_PFC_UP_ATTR_6]   = {.type = NLA_U8},
> 	[DCB_PFC_UP_ATTR_7]   = {.type = NLA_U8},
> 	[DCB_PFC_UP_ATTR_ALL] = {.type = NLA_FLAG},
> };
> 
> static const struct nla_policy dcbnl_bcn_nest[...] = {
> 	[DCB_BCN_ATTR_RP_0]         = {.type = NLA_U8},
> 	[DCB_BCN_ATTR_RP_1]         = {.type = NLA_U8},
> 	[DCB_BCN_ATTR_RP_2]         = {.type = NLA_U8},
> 	[DCB_BCN_ATTR_RP_3]         = {.type = NLA_U8},
> 	[DCB_BCN_ATTR_RP_4]         = {.type = NLA_U8},
> 	[DCB_BCN_ATTR_RP_5]         = {.type = NLA_U8},
> 	[DCB_BCN_ATTR_RP_6]         = {.type = NLA_U8},
> 	[DCB_BCN_ATTR_RP_7]         = {.type = NLA_U8},
> 	[DCB_BCN_ATTR_RP_ALL]       = {.type = NLA_FLAG},
> 	// from here is somewhat different
> 	[DCB_BCN_ATTR_BCNA_0]       = {.type = NLA_U32},
>         ...
>         [DCB_BCN_ATTR_ALL]          = {.type = NLA_FLAG},
> };
> 
> Therefore, the current code is buggy and this
> nla_parse_nested_deprecated could overflow the dcbnl_pfc_up_nest and use
> the adjacent nla_policy to parse attributes from DCB_BCN_ATTR_BCNA_0.
> 
> This patch use correct dcbnl_bcn_nest policy to parse the
> tb[DCB_ATTR_BCN] nested TLV.
> 
> Fixes: 859ee3c43812 ("DCB: Add support for DCB BCN")
> Signed-off-by: Lin Ma <linma@zju.edu.cn>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net] net: dcb: choose correct policy to parse DCB_ATTR_BCN

[Markus Elfring]

…
> This patch use correct dcbnl_bcn_nest policy to parse the
> tb[DCB_ATTR_BCN] nested TLV.

Are imperative change descriptions still preferred?

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc3#n94

Regards,
Markus

Re: [PATCH net] net: dcb: choose correct policy to parse DCB_ATTR_BCN

[Lin Ma]

Hello Markus,

> 
> …
> > This patch use correct dcbnl_bcn_nest policy to parse the
> > tb[DCB_ATTR_BCN] nested TLV.
> 
> Are imperative change descriptions still preferred?
> 
> See also:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc3#n94
> 
> Regards,
> Markus

Yeah, thanks for reminding me. I haven't been paying attention to that and I will remember this ever since. :D

Regards,
Lin

Re: [PATCH net] net: dcb: choose correct policy to parse DCB_ATTR_BCN

[Dan Carpenter]

On Tue, Aug 01, 2023 at 09:34:17AM +0800, Lin Ma wrote:
> Hello Markus,
> 
> > 
> > …
> > > This patch use correct dcbnl_bcn_nest policy to parse the
> > > tb[DCB_ATTR_BCN] nested TLV.
> > 
> > Are imperative change descriptions still preferred?
> > 
> > See also:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc3#n94
> > 
> > Regards,
> > Markus
> 
> Yeah, thanks for reminding me. I haven't been paying attention to that
> and I will remember this ever since. :D

Simon reviewed the patch already.  Don't listen to Markus.  He's banned
from vger.

https://lore.kernel.org/all/2023073123-poser-panhandle-1cb7@gregkh/

regards,
dan carpenter

Re: [PATCH net] net: dcb: choose correct policy to parse DCB_ATTR_BCN

[Lin Ma]

Hello Dan,

> 
> Simon reviewed the patch already.  Don't listen to Markus.  He's banned
> from vger.
> 
> https://lore.kernel.org/all/2023073123-poser-panhandle-1cb7@gregkh/
> 
> regards,
> dan carpenter

Ooooops, I never thought of it like this. I will take note of that :).

Thanks
Lin

Re: net: dcb: Communication challenges for patch reviews?

[Markus Elfring]

> Simon reviewed the patch already.

It seems that some reviewers do not care so much about the recommended application
of imperative mood for improved change descriptions.


> Don't listen to Markus.

I hope that further suggestions (including remaining patches) will get more
development attention so that the change acceptance will evolve accordingly.


> He's banned from vger.

Is this historic action a questionable side effect of known recurring
communication difficulties?

How many contributors are blocked so far?

There are also participants who are still more open to some change ideas.


> https://lore.kernel.org/all/2023073123-poser-panhandle-1cb7@gregkh/

I imagine that the “intelligence” can be improved also for
“the semi-friendly patch-bot of Greg Kroah-Hartman”.

How “nonsensical or otherwise pointless” do you find review comments
which remind requirements from the Linux development documentation
for a discussed patch?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc4#n3

Regards,
Markus

Re: net: dcb: Communication challenges for patch reviews?

[Dan Carpenter]

I'm not trying to be mean.  I don't go out looking for Markus's emails,
but he often adds kernel-janitors to the CC list.  Kernel janitors is a
vger list so he's banned but we still see the responses to his emails.

In recent months probably seven maintainers have asked him over and over
(maybe 20 times?) to stop with this nonsense.  So he knew he shouldn't
have asked Lin Ma to redo the patch.

regards,
dan carpenter

Re: net: dcb: Communication challenges for patch reviews?

[Markus Elfring]

> In recent months probably seven maintainers have asked him over and over
> (maybe 20 times?) to stop with this nonsense.

Strong responses can occasionally occur if you dare to present special
suggestions for patch reviews according to components in several subsystems.
I tend to interpret hints for improved adherence to Linux development requirements
as helpful so far.


>                                                So he knew he shouldn't
> have asked Lin Ma to redo the patch.

This contributor could just pick another change idea up in a constructive way.

Regards,
Markus