* [PATCH net-next v1] net: mctp: use deprecated parser in mctp_set_link_af
@ 2023-12-28 7:02 Lin Ma
2024-01-04 1:47 ` Jakub Kicinski
0 siblings, 1 reply; 5+ messages in thread
From: Lin Ma @ 2023-12-28 7:02 UTC (permalink / raw)
To: jk, matt, davem, edumazet, kuba, pabeni, netdev, linux-kernel; +Cc: Lin Ma
In mctp set_link_af implementation `mctp_set_link_af`, it uses strict
parser nla_parse_nested to parse the nested attribute. This is fine in
most cases but not here, as the rtnetlink uses *bad magic* in setlink
code, see code snippet in function `do_setlink`.
nla_for_each_nested(af, tb[IFLA_AF_SPEC], rem) {
const struct rtnl_af_ops *af_ops;
BUG_ON(!(af_ops = rtnl_af_lookup(nla_type(af)))); <= (1)
err = af_ops->set_link_af(dev, af, extack); <= (2)
That is, in line (1), the attribute type of af will used to look up the
af_ops, and for MCTP case will use AF_MCTP here to get mctp_af_ops.
Therefore, the attribute with type AF_MCTP will never survive in the
check within the nla_parse_nested.
if (!(nla->nla_type & NLA_F_NESTED)) { <= nla_type is AF_MCTP
NL_SET_ERR_MSG_ATTR(extack, nla, "NLA_F_NESTED is missing");
return -EINVAL; <= always invalid
}
For other set_link_af users IPV4 and IPV6 both make a trick here by
using nla_parse_nested_deprecated, which will check the NLA_F_NESTED
then able to use this type field as family value. This patch simply port
the MCTP code also to deprecated parser to make it work.
Signed-off-by: Lin Ma <linma@zju.edu.cn>
---
net/mctp/device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/mctp/device.c b/net/mctp/device.c
index acb97b257428..226c8e3ed85f 100644
--- a/net/mctp/device.c
+++ b/net/mctp/device.c
@@ -400,8 +400,8 @@ static int mctp_set_link_af(struct net_device *dev, const struct nlattr *attr,
struct mctp_dev *mdev;
int rc;
- rc = nla_parse_nested(tb, IFLA_MCTP_MAX, attr, ifla_af_mctp_policy,
- NULL);
+ rc = nla_parse_nested_deprecated(tb, IFLA_MCTP_MAX, attr, ifla_af_mctp_policy,
+ NULL);
if (rc)
return rc;
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v1] net: mctp: use deprecated parser in mctp_set_link_af
2023-12-28 7:02 [PATCH net-next v1] net: mctp: use deprecated parser in mctp_set_link_af Lin Ma
@ 2024-01-04 1:47 ` Jakub Kicinski
2024-01-04 6:32 ` Lin Ma
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Kicinski @ 2024-01-04 1:47 UTC (permalink / raw)
To: Lin Ma; +Cc: jk, matt, davem, edumazet, pabeni, netdev, linux-kernel
On Thu, 28 Dec 2023 15:02:58 +0800 Lin Ma wrote:
> In mctp set_link_af implementation `mctp_set_link_af`, it uses strict
> parser nla_parse_nested to parse the nested attribute. This is fine in
> most cases but not here, as the rtnetlink uses *bad magic* in setlink
> code, see code snippet in function `do_setlink`.
>
> nla_for_each_nested(af, tb[IFLA_AF_SPEC], rem) {
> const struct rtnl_af_ops *af_ops;
> BUG_ON(!(af_ops = rtnl_af_lookup(nla_type(af)))); <= (1)
> err = af_ops->set_link_af(dev, af, extack); <= (2)
>
> That is, in line (1), the attribute type of af will used to look up the
> af_ops, and for MCTP case will use AF_MCTP here to get mctp_af_ops.
> Therefore, the attribute with type AF_MCTP will never survive in the
> check within the nla_parse_nested.
>
> if (!(nla->nla_type & NLA_F_NESTED)) { <= nla_type is AF_MCTP
> NL_SET_ERR_MSG_ATTR(extack, nla, "NLA_F_NESTED is missing");
> return -EINVAL; <= always invalid
> }
>
> For other set_link_af users IPV4 and IPV6 both make a trick here by
> using nla_parse_nested_deprecated, which will check the NLA_F_NESTED
> then able to use this type field as family value. This patch simply port
> the MCTP code also to deprecated parser to make it work.
Did you test this? It's a suspiciously detailed and yet seemingly
incorrect explanation..
--
pv-bot: s
pw-bot: cr
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v1] net: mctp: use deprecated parser in mctp_set_link_af
2024-01-04 1:47 ` Jakub Kicinski
@ 2024-01-04 6:32 ` Lin Ma
2024-01-04 6:43 ` Jeremy Kerr
0 siblings, 1 reply; 5+ messages in thread
From: Lin Ma @ 2024-01-04 6:32 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: jk, matt, davem, edumazet, pabeni, netdev, linux-kernel
Hello Jakub,
> >
> > For other set_link_af users IPV4 and IPV6 both make a trick here by
> > using nla_parse_nested_deprecated, which will check the NLA_F_NESTED
> > then able to use this type field as family value. This patch simply port
> > the MCTP code also to deprecated parser to make it work.
>
> Did you test this? It's a suspiciously detailed and yet seemingly
> incorrect explanation..
Yes, I hacked the iproute tool and add below code
https://github.com/iproute2/iproute2/blob/main/ip/iplink.c#L801
853a854,861
> } else if (matches(*argv, "mctptest") == 0) {
> struct rtattr *afs, *afsmctp;
> afs = addattr_nest(&req->n, sizeof(*req), IFLA_AF_SPEC);
> afsmctp = addattr_nest(&req->n, sizeof(*req), AF_MCTP);
> addattr32(&req->n, sizeof(*req),
> IFLA_MCTP_NET, 0);
> addattr_nest_end(&req->n, afsmctp);
> addattr_nest_end(&req->n, afs);
according to the implementation of the `addrgenmode`.
In short, the package format should be like
L T L T
+---+--------------+---+---------+-------------
| | IFLA_AF_SPEC | | AF_MCTP | .....
+---+--------------+---+---------+-------------
^
|
af
Therefore, the code
nla_for_each_nested(af, tb[IFLA_AF_SPEC], rem) {
const struct rtnl_af_ops *af_ops;
BUG_ON(!(af_ops = rtnl_af_lookup(nla_type(af)))); <= (1)
err = af_ops->set_link_af(dev, af, extack); <= (2)
(1) will get AF_MCTP with nla_type(...) call, and (2) will then passing `af` to
`mctp_set_link_af`, which will expect the `NLA_F_NESTED` flag.
With the hacked iproute tools, simple type command like
`ip link set eth0 mtu 1024 mctptest` will hit the function mctp_set_link_af and
with my debugger, I found that the nla_parse_nested always return error.
Another elegant solution is to change how (1) calls rtnl_af_lookup, we can introduce
another family attribute and avoid to use nla_type(..) which is so suspicious.
But this will have to change also other set_link_af implementations (ipv4, ipv6).
> --
> pv-bot: s
> pw-bot: cr
Regards
Lin
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v1] net: mctp: use deprecated parser in mctp_set_link_af
2024-01-04 6:32 ` Lin Ma
@ 2024-01-04 6:43 ` Jeremy Kerr
2024-01-04 7:04 ` Lin Ma
0 siblings, 1 reply; 5+ messages in thread
From: Jeremy Kerr @ 2024-01-04 6:43 UTC (permalink / raw)
To: Lin Ma, Jakub Kicinski
Cc: matt, davem, edumazet, pabeni, netdev, linux-kernel
Hi Lin,
> (1) will get AF_MCTP with nla_type(...) call, and (2) will then
> passing `af` to `mctp_set_link_af`, which will expect the
> `NLA_F_NESTED` flag.
You could set that flag? It's not exclusive with the attribute type.
From netlink.h:
/*
* nla_type (16 bits)
* +---+---+-------------------------------+
* | N | O | Attribute Type |
* +---+---+-------------------------------+
* N := Carries nested attributes
* O := Payload stored in network byte order
*
* Note: The N and O flag are mutually exclusive.
*/
Which is what we do with existing userspace:
https://github.com/CodeConstruct/mctp/blob/6aa4b052d/src/mctp.c#L569C1-L569C1
Cheers,
Jeremy
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next v1] net: mctp: use deprecated parser in mctp_set_link_af
2024-01-04 6:43 ` Jeremy Kerr
@ 2024-01-04 7:04 ` Lin Ma
0 siblings, 0 replies; 5+ messages in thread
From: Lin Ma @ 2024-01-04 7:04 UTC (permalink / raw)
To: Jeremy Kerr
Cc: Jakub Kicinski, matt, davem, edumazet, pabeni, netdev, linux-kernel
Hello Jeremy,
>
> Hi Lin,
>
> > (1) will get AF_MCTP with nla_type(...) call, and (2) will then
> > passing `af` to `mctp_set_link_af`, which will expect the
> > `NLA_F_NESTED` flag.
>
> You could set that flag? It's not exclusive with the attribute type.
> From netlink.h:
>
> /*
> * nla_type (16 bits)
> * +---+---+-------------------------------+
> * | N | O | Attribute Type |
> * +---+---+-------------------------------+
> * N := Carries nested attributes
> * O := Payload stored in network byte order
> *
> * Note: The N and O flag are mutually exclusive.
> */
>
> Which is what we do with existing userspace:
>
> https://github.com/CodeConstruct/mctp/blob/6aa4b052d/src/mctp.c#L569C1-L569C1
>
> Cheers,
Oops, my bad. Just know that the nla_type will mask with `NLA_TYPE_MASK`.
Sorry for the bother. T^T
>
>
> Jeremy
Thanks
Lin
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-01-04 7:05 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-28 7:02 [PATCH net-next v1] net: mctp: use deprecated parser in mctp_set_link_af Lin Ma
2024-01-04 1:47 ` Jakub Kicinski
2024-01-04 6:32 ` Lin Ma
2024-01-04 6:43 ` Jeremy Kerr
2024-01-04 7:04 ` Lin Ma
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).