Re: [PATCH net-next v1] net: mctp: use deprecated parser in mctp_set_link_af

[Lin Ma]

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