| Message ID | 20230731045216.3779420-1-linma@zju.edu.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v1] net: dcb: choose correct policy to parse DCB_ATTR_BCN | expand |
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>
… > 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
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
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
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
> 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
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
> 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
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;
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(-)