| Message ID | d94672114f94dc029fdb4a721d54c200521e180d.1686562577.git.dcaratti@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [mptcp-next,v2] mptcp: convert netlink from small_ops to ops | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | warning | Build error with: make C=1 net/mptcp/pm_netlink.o |
| matttbe/checkpatch | warning | total: 0 errors, 0 warnings, 1 checks, 379 lines checked |
| matttbe/KVM_Validation__normal__except_selftest_mptcp_join_ | warning | Unstable: 2 failed test(s): selftest_simult_flows selftest_userspace_pm |
| matttbe/KVM_Validation__normal__only_selftest_mptcp_join_ | success | Success! ✅ |
| matttbe/KVM_Validation__debug__except_selftest_mptcp_join_ | warning | Unstable: 1 failed test(s): selftest_userspace_pm |
| matttbe/KVM_Validation__debug__only_selftest_mptcp_join_ | success | Success! ✅ |
Hi Davide, Thank you for your modifications, that's great! But sadly, our CI spotted some issues with it when trying to build it. You can find more details there: https://patchwork.kernel.org/project/mptcp/patch/d94672114f94dc029fdb4a721d54c200521e180d.1686562577.git.dcaratti@redhat.com/ https://github.com/multipath-tcp/mptcp_net-next/actions/runs/5242115507 Status: failure Initiator: MPTCPimporter Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/7c8f791a2d25 Feel free to reply to this email if you cannot access logs, if you need some support to fix the error, if this doesn't seem to be caused by your modifications or if the error is a false positive one. Cheers, MPTCP GH Action bot Bot operated by Matthieu Baerts (Tessares)
Hi Davide, Overall LGTM, there are just a few small hints see below. On Mon, 2023-06-12 at 11:39 +0200, Davide Caratti wrote: > this prepares MPTCP control plane to be described as YAML spec. I think the commit message should be expanded, alike """ After the change the mptcp protocol will use per op policy. Update the commands implementation to pass the relevant policy all the way down to the actual impl. """ Or whatever makes really sense ;) > > Link: https://github.com/multipath-tcp/mptcp_net-next/issues/340 > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > --- > net/mptcp/pm_netlink.c | 259 +++++++++++++++++++++++++++++---------- > net/mptcp/pm_userspace.c | 2 +- > net/mptcp/protocol.h | 2 +- > 3 files changed, 193 insertions(+), 70 deletions(-) > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c > index ba1406e601e4..b7c7e5f81fd1 100644 > --- a/net/mptcp/pm_netlink.c > +++ b/net/mptcp/pm_netlink.c > @@ -47,6 +47,103 @@ struct pm_nl_pernet { > #define MPTCP_PM_ADDR_MAX 8 > #define ADD_ADDR_RETRANS_MAX 3 > > +static const struct nla_policy *mptcp_nl_cmd2policy(int cmd); > + > +#define mptcp_pm_endpoint_nl_policy mptcp_pm_add_addr_nl_policy > + > +/* MPTCP_PM_CMD_ADD_ADDR - do */ > +static const struct nla_policy mptcp_pm_add_addr_nl_policy[MPTCP_PM_ADDR_ATTR_IF_IDX + 1] = { > + [MPTCP_PM_ADDR_ATTR_FAMILY] = { .type = NLA_U16, }, > + [MPTCP_PM_ADDR_ATTR_ADDR4] = { .type = NLA_U32, }, > + [MPTCP_PM_ADDR_ATTR_ADDR6] = { .len = 16, }, > + [MPTCP_PM_ADDR_ATTR_ID] = { .type = NLA_U8, }, > + [MPTCP_PM_ADDR_ATTR_PORT] = { .type = NLA_U16, }, > + [MPTCP_PM_ADDR_ATTR_IF_IDX] = { .type = NLA_S32, }, > + [MPTCP_PM_ADDR_ATTR_FLAGS] = { .type = NLA_U32, }, > +}; > + > +/* MPTCP_PM_CMD_DEL_ADDR - do */ > +static const struct nla_policy mptcp_pm_del_addr_nl_policy[MPTCP_PM_ADDR_ATTR_IF_IDX + 1] = { > + [MPTCP_PM_ADDR_ATTR_FAMILY] = { .type = NLA_U16, }, > + [MPTCP_PM_ADDR_ATTR_ADDR4] = { .type = NLA_U32, }, > + [MPTCP_PM_ADDR_ATTR_ADDR6] = { .len = 16, }, > + [MPTCP_PM_ADDR_ATTR_ID] = { .type = NLA_U8, }, > + [MPTCP_PM_ADDR_ATTR_PORT] = { .type = NLA_U16, }, > + [MPTCP_PM_ADDR_ATTR_IF_IDX] = { .type = NLA_S32, }, > + [MPTCP_PM_ADDR_ATTR_FLAGS] = { .type = NLA_U32, }, > +}; > + > +/* MPTCP_PM_CMD_GET_ADDR - do */ > +static const struct nla_policy mptcp_pm_get_addr_nl_policy[MPTCP_PM_ADDR_ATTR_IF_IDX + 1] = { > + [MPTCP_PM_ADDR_ATTR_FAMILY] = { .type = NLA_U16, }, > + [MPTCP_PM_ADDR_ATTR_ADDR4] = { .type = NLA_U32, }, > + [MPTCP_PM_ADDR_ATTR_ADDR6] = { .len = 16, }, > + [MPTCP_PM_ADDR_ATTR_ID] = { .type = NLA_U8, }, > + [MPTCP_PM_ADDR_ATTR_PORT] = { .type = NLA_U16, }, > + [MPTCP_PM_ADDR_ATTR_IF_IDX] = { .type = NLA_S32, }, > + [MPTCP_PM_ADDR_ATTR_FLAGS] = { .type = NLA_U32, }, > +}; > + > +/* MPTCP_PM_CMD_FLUSH_ADDRS - do */ > +static const struct nla_policy mptcp_pm_flush_addrs_nl_policy[MPTCP_PM_ADDR_ATTR_IF_IDX + 1] = { > + [MPTCP_PM_ADDR_ATTR_FAMILY] = { .type = NLA_U16, }, > + [MPTCP_PM_ADDR_ATTR_ADDR4] = { .type = NLA_U32, }, > + [MPTCP_PM_ADDR_ATTR_ADDR6] = { .len = 16, }, > + [MPTCP_PM_ADDR_ATTR_ID] = { .type = NLA_U8, }, > + [MPTCP_PM_ADDR_ATTR_PORT] = { .type = NLA_U16, }, > + [MPTCP_PM_ADDR_ATTR_IF_IDX] = { .type = NLA_S32, }, > + [MPTCP_PM_ADDR_ATTR_FLAGS] = { .type = NLA_U32, }, > +}; Out of sheer ignorance, to we need to declare 4 different structs with the same contents? Can't we reuse a single one 4 times? > + > +/* MPTCP_PM_CMD_SET_LIMITS - do */ > +static const struct nla_policy mptcp_pm_set_limits_nl_policy[MPTCP_PM_ATTR_SUBFLOWS + 1] = { > + [MPTCP_PM_ATTR_RCV_ADD_ADDRS] = { .type = NLA_U32, }, > + [MPTCP_PM_ATTR_SUBFLOWS] = { .type = NLA_U32, }, > +}; > + > +/* MPTCP_PM_CMD_GET_LIMITS - do */ > +static const struct nla_policy mptcp_pm_get_limits_nl_policy[MPTCP_PM_ATTR_SUBFLOWS + 1] = { > + [MPTCP_PM_ATTR_RCV_ADD_ADDRS] = { .type = NLA_U32, }, > + [MPTCP_PM_ATTR_SUBFLOWS] = { .type = NLA_U32, }, > +}; > + > +/* MPTCP_PM_CMD_SET_FLAGS - do */ > +static const struct nla_policy mptcp_pm_set_flags_nl_policy[MPTCP_PM_ADDR_ATTR_IF_IDX + 1] = { > + [MPTCP_PM_ADDR_ATTR_FAMILY] = { .type = NLA_U16, }, > + [MPTCP_PM_ADDR_ATTR_ADDR4] = { .type = NLA_U32, }, > + [MPTCP_PM_ADDR_ATTR_ADDR6] = { .len = 16, }, > + [MPTCP_PM_ADDR_ATTR_ID] = { .type = NLA_U8, }, > + [MPTCP_PM_ADDR_ATTR_PORT] = { .type = NLA_U16, }, > + [MPTCP_PM_ADDR_ATTR_IF_IDX] = { .type = NLA_S32, }, > + [MPTCP_PM_ADDR_ATTR_FLAGS] = { .type = NLA_U32, }, > +}; > + > +/* MPTCP_PM_CMD_ANNOUNCE - do */ > +static const struct nla_policy mptcp_pm_announce_nl_policy[MPTCP_PM_ATTR_TOKEN + 1] = { > + [MPTCP_PM_ATTR_TOKEN] = { .type = NLA_U32, }, > + [MPTCP_PM_ATTR_ADDR] = NLA_POLICY_NESTED(mptcp_pm_endpoint_nl_policy), > +}; > + > +/* MPTCP_PM_CMD_REMOVE - do */ > +static const struct nla_policy mptcp_pm_remove_nl_policy[MPTCP_PM_ATTR_LOC_ID + 1] = { > + [MPTCP_PM_ATTR_TOKEN] = { .type = NLA_U32, }, > + [MPTCP_PM_ATTR_LOC_ID] = { .type = NLA_U8, }, > +}; > + > +/* MPTCP_PM_CMD_SUBFLOW_CREATE - do */ > +static const struct nla_policy mptcp_pm_subflow_create_nl_policy[MPTCP_PM_ATTR_ADDR_REMOTE + 1] = { > + [MPTCP_PM_ATTR_ADDR_REMOTE] = NLA_POLICY_NESTED(mptcp_pm_endpoint_nl_policy), > + [MPTCP_PM_ATTR_TOKEN] = { .type = NLA_U32, }, > + [MPTCP_PM_ATTR_ADDR] = NLA_POLICY_NESTED(mptcp_pm_endpoint_nl_policy), > +}; > + > +/* MPTCP_PM_CMD_SUBFLOW_DESTROY - do */ > +static const struct nla_policy mptcp_pm_subflow_destroy_nl_policy[MPTCP_PM_ATTR_ADDR_REMOTE + 1] = { > + [MPTCP_PM_ATTR_ADDR_REMOTE] = NLA_POLICY_NESTED(mptcp_pm_endpoint_nl_policy), > + [MPTCP_PM_ATTR_TOKEN] = { .type = NLA_U32, }, > + [MPTCP_PM_ATTR_ADDR] = NLA_POLICY_NESTED(mptcp_pm_endpoint_nl_policy), > +}; > + > static struct pm_nl_pernet *pm_nl_get_pernet(const struct net *net) > { > return net_generic(net, pm_nl_pernet_id); > @@ -1099,29 +1196,6 @@ static const struct genl_multicast_group mptcp_pm_mcgrps[] = { > }, > }; > > -static const struct nla_policy > -mptcp_pm_addr_policy[MPTCP_PM_ADDR_ATTR_MAX + 1] = { > - [MPTCP_PM_ADDR_ATTR_FAMILY] = { .type = NLA_U16, }, > - [MPTCP_PM_ADDR_ATTR_ID] = { .type = NLA_U8, }, > - [MPTCP_PM_ADDR_ATTR_ADDR4] = { .type = NLA_U32, }, > - [MPTCP_PM_ADDR_ATTR_ADDR6] = > - NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)), > - [MPTCP_PM_ADDR_ATTR_PORT] = { .type = NLA_U16 }, > - [MPTCP_PM_ADDR_ATTR_FLAGS] = { .type = NLA_U32 }, > - [MPTCP_PM_ADDR_ATTR_IF_IDX] = { .type = NLA_S32 }, > -}; > - > -static const struct nla_policy mptcp_pm_policy[MPTCP_PM_ATTR_MAX + 1] = { > - [MPTCP_PM_ATTR_ADDR] = > - NLA_POLICY_NESTED(mptcp_pm_addr_policy), > - [MPTCP_PM_ATTR_RCV_ADD_ADDRS] = { .type = NLA_U32, }, > - [MPTCP_PM_ATTR_SUBFLOWS] = { .type = NLA_U32, }, > - [MPTCP_PM_ATTR_TOKEN] = { .type = NLA_U32, }, > - [MPTCP_PM_ATTR_LOC_ID] = { .type = NLA_U8, }, > - [MPTCP_PM_ATTR_ADDR_REMOTE] = > - NLA_POLICY_NESTED(mptcp_pm_addr_policy), > -}; > - > void mptcp_pm_nl_subflow_chk_stale(const struct mptcp_sock *msk, struct sock *ssk) > { > struct mptcp_subflow_context *iter, *subflow = mptcp_subflow_ctx(ssk); > @@ -1171,6 +1245,7 @@ static int mptcp_pm_family_to_addr(int family) > static int mptcp_pm_parse_pm_addr_attr(struct nlattr *tb[], > const struct nlattr *attr, > struct genl_info *info, > + const struct nla_policy *nla_policy, > struct mptcp_addr_info *addr, > bool require_family) > { > @@ -1183,7 +1258,8 @@ static int mptcp_pm_parse_pm_addr_attr(struct nlattr *tb[], > > /* no validation needed - was already done via nested policy */ > err = nla_parse_nested_deprecated(tb, MPTCP_PM_ADDR_ATTR_MAX, attr, > - mptcp_pm_addr_policy, info->extack); > + nla_policy, > + info->extack); Minor nit: I think we can use a single line for both 'nla_policy' and 'info->extack);' > if (err) > return err; > > @@ -1236,19 +1312,22 @@ int mptcp_pm_parse_addr(struct nlattr *attr, struct genl_info *info, > > memset(addr, 0, sizeof(*addr)); > > - return mptcp_pm_parse_pm_addr_attr(tb, attr, info, addr, true); > + return mptcp_pm_parse_pm_addr_attr(tb, attr, info, mptcp_pm_endpoint_nl_policy, addr, true); > } > > int mptcp_pm_parse_entry(struct nlattr *attr, struct genl_info *info, > - bool require_family, > + int cmd, bool require_family, > struct mptcp_pm_addr_entry *entry) > { > struct nlattr *tb[MPTCP_PM_ADDR_ATTR_MAX + 1]; > + const struct nla_policy *nla_policy = mptcp_nl_cmd2policy(cmd); > int err; Nit: reverse x-mas tree above > > memset(entry, 0, sizeof(*entry)); > > - err = mptcp_pm_parse_pm_addr_attr(tb, attr, info, &entry->addr, require_family); > + err = mptcp_pm_parse_pm_addr_attr(tb, attr, info, > + nla_policy, > + &entry->addr, require_family); Nit: tb, attr, info, nla_policy, > if (err) > return err; > > @@ -1305,7 +1384,7 @@ static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info) > struct mptcp_pm_addr_entry addr, *entry; > int ret; > > - ret = mptcp_pm_parse_entry(attr, info, true, &addr); > + ret = mptcp_pm_parse_entry(attr, info, MPTCP_PM_CMD_ADD_ADDR, true, &addr); Can we pass to mptcp_pm_parse_entry() directly the relevant policy instead of the cmd? it will avoid the need for mptcp_nl_cmd2policy() and possibly for the 'duplicated' definitions above. Otherwise LGTM, thanks! Paolo
Hi Davide, Thank you for your modifications, that's great! Our CI did some validations and here is its report: - KVM Validation: normal (except selftest_mptcp_join): - Unstable: 2 failed test(s): selftest_simult_flows selftest_userspace_pm
hello Paolo, thanks for looking at this! On Mon, Jun 12, 2023 at 12:15:59PM +0200, Paolo Abeni wrote: > Hi Davide, > > Overall LGTM, there are just a few small hints see below. > > On Mon, 2023-06-12 at 11:39 +0200, Davide Caratti wrote: > > this prepares MPTCP control plane to be described as YAML spec. > > I think the commit message should be expanded, alike > > """ > After the change the mptcp protocol will use per op policy. Update > the commands implementation to pass the relevant policy all the way > down to the actual impl. > """ > > Or whatever makes really sense ;) Ok! [...] > > > > + > > +/* MPTCP_PM_CMD_FLUSH_ADDRS - do */ > > +static const struct nla_policy mptcp_pm_flush_addrs_nl_policy[MPTCP_PM_ADDR_ATTR_IF_IDX + 1] = { > > + [MPTCP_PM_ADDR_ATTR_FAMILY] = { .type = NLA_U16, }, > > + [MPTCP_PM_ADDR_ATTR_ADDR4] = { .type = NLA_U32, }, > > + [MPTCP_PM_ADDR_ATTR_ADDR6] = { .len = 16, }, > > + [MPTCP_PM_ADDR_ATTR_ID] = { .type = NLA_U8, }, > > + [MPTCP_PM_ADDR_ATTR_PORT] = { .type = NLA_U16, }, > > + [MPTCP_PM_ADDR_ATTR_IF_IDX] = { .type = NLA_S32, }, > > + [MPTCP_PM_ADDR_ATTR_FLAGS] = { .type = NLA_U32, }, > > +}; > > Out of sheer ignorance, to we need to declare 4 different structs with > the same contents? Can't we reuse a single one 4 times? good point. Well, "per-op" implies (theoretically) that validation of the same attribute set can be different per-op: for example, you might want to remove an endpoint with 'id' equal to 0, but you can't add one having id = 0. The code I added is taken from the output of ynl-cli, and you might have noticed that I added the #define trick for "endpoint" because the tool didn't generate any policy for the nested attribute... Honestly I thought it would have been sufficient to specify "aliases" in request/replies, like "&add_addr_attrs" and "*add_addr_attrs" : <...> - name: add_addr doc: Add endpoint attribute-set: endpoint dont-validate: [ strict ] flags: [ uns-admin-perm ] do: request: &add_addr_attrs attributes: - family - addr4 - addr6 - id - port - if_idx - flags - name: del_addr doc: Delete endpoint attribute-set: endpoint dont-validate: [ strict ] flags: [ uns-admin-perm ] do: request: *add_addr_attrs <...> but this generated 2 separate struct of the same type, namely mptcp_pm_add_addr_nl_policy and mptcp_pm_del_addr_nl_policy. [...] > > @@ -1183,7 +1258,8 @@ static int mptcp_pm_parse_pm_addr_attr(struct nlattr *tb[], > > > > /* no validation needed - was already done via nested policy */ > > err = nla_parse_nested_deprecated(tb, MPTCP_PM_ADDR_ATTR_MAX, attr, > > - mptcp_pm_addr_policy, info->extack); > > + nla_policy, > > + info->extack); > > Minor nit: I think we can use a single line for both 'nla_policy' and > 'info->extack);' yes, sure, will fix this in v4 [...] > > > @@ -1236,19 +1312,22 @@ int mptcp_pm_parse_addr(struct nlattr *attr, struct genl_info *info, > > > > memset(addr, 0, sizeof(*addr)); > > > > - return mptcp_pm_parse_pm_addr_attr(tb, attr, info, addr, true); > > + return mptcp_pm_parse_pm_addr_attr(tb, attr, info, mptcp_pm_endpoint_nl_policy, addr, true); > > } > > > > int mptcp_pm_parse_entry(struct nlattr *attr, struct genl_info *info, > > - bool require_family, > > + int cmd, bool require_family, > > struct mptcp_pm_addr_entry *entry) > > { > > struct nlattr *tb[MPTCP_PM_ADDR_ATTR_MAX + 1]; > > + const struct nla_policy *nla_policy = mptcp_nl_cmd2policy(cmd); > > int err; > > Nit: reverse x-mas tree above sure, will fix it in v4 > > > > > memset(entry, 0, sizeof(*entry)); > > > > - err = mptcp_pm_parse_pm_addr_attr(tb, attr, info, &entry->addr, require_family); > > + err = mptcp_pm_parse_pm_addr_attr(tb, attr, info, > > + nla_policy, > > + &entry->addr, require_family); > > Nit: > tb, attr, info, nla_policy, sure, will fix it in v4 > > @@ -1305,7 +1384,7 @@ static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info) > > struct mptcp_pm_addr_entry addr, *entry; > > int ret; > > > > - ret = mptcp_pm_parse_entry(attr, info, true, &addr); > > + ret = mptcp_pm_parse_entry(attr, info, MPTCP_PM_CMD_ADD_ADDR, true, &addr); > > Can we pass to mptcp_pm_parse_entry() directly the relevant policy > instead of the cmd? it will avoid the need for mptcp_nl_cmd2policy() > and possibly for the 'duplicated' definitions above. this will avoid the need for mptcp_nl_cmd2policy(), but it won't probably eliminate the dups in the long term, when yamlification will be in place. (I need to confirm this by looking again at the tool, will let you know).
On Mon, 2023-06-12 at 15:54 +0200, Davide Caratti wrote: > On Mon, Jun 12, 2023 at 12:15:59PM +0200, Paolo Abeni wrote: > > On Mon, 2023-06-12 at 11:39 +0200, Davide Caratti wrote: > > > > > @@ -1305,7 +1384,7 @@ static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info) > > > struct mptcp_pm_addr_entry addr, *entry; > > > int ret; > > > > > > - ret = mptcp_pm_parse_entry(attr, info, true, &addr); > > > + ret = mptcp_pm_parse_entry(attr, info, MPTCP_PM_CMD_ADD_ADDR, true, &addr); > > > > Can we pass to mptcp_pm_parse_entry() directly the relevant policy > > instead of the cmd? it will avoid the need for mptcp_nl_cmd2policy() > > and possibly for the 'duplicated' definitions above. > > this will avoid the need for mptcp_nl_cmd2policy(), but it won't probably > eliminate the dups in the long term, when yamlification will be in place. > (I need to confirm this by looking again at the tool, will let you know). Whoops, I did not see the V3 when I first commented here, sorry! (well, mid-air collision mostily ;) I *think* let the yaml-ification introduce the code duplication would be better, as that code is automatically generated, we can allows dups there. Also, if the tool will change/improve, such dups *could* be automatically-removed in the future. Unless is causing some problem/functional issue/unneeded complexity or complication, I would avoid adding the dups here. Otherwise, if any of the above is true, let's add the dups and whatever! /P
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c index ba1406e601e4..b7c7e5f81fd1 100644 --- a/net/mptcp/pm_netlink.c +++ b/net/mptcp/pm_netlink.c @@ -47,6 +47,103 @@ struct pm_nl_pernet { #define MPTCP_PM_ADDR_MAX 8 #define ADD_ADDR_RETRANS_MAX 3 +static const struct nla_policy *mptcp_nl_cmd2policy(int cmd); + +#define mptcp_pm_endpoint_nl_policy mptcp_pm_add_addr_nl_policy + +/* MPTCP_PM_CMD_ADD_ADDR - do */ +static const struct nla_policy mptcp_pm_add_addr_nl_policy[MPTCP_PM_ADDR_ATTR_IF_IDX + 1] = { + [MPTCP_PM_ADDR_ATTR_FAMILY] = { .type = NLA_U16, }, + [MPTCP_PM_ADDR_ATTR_ADDR4] = { .type = NLA_U32, }, + [MPTCP_PM_ADDR_ATTR_ADDR6] = { .len = 16, }, + [MPTCP_PM_ADDR_ATTR_ID] = { .type = NLA_U8, }, + [MPTCP_PM_ADDR_ATTR_PORT] = { .type = NLA_U16, }, + [MPTCP_PM_ADDR_ATTR_IF_IDX] = { .type = NLA_S32, }, + [MPTCP_PM_ADDR_ATTR_FLAGS] = { .type = NLA_U32, }, +}; + +/* MPTCP_PM_CMD_DEL_ADDR - do */ +static const struct nla_policy mptcp_pm_del_addr_nl_policy[MPTCP_PM_ADDR_ATTR_IF_IDX + 1] = { + [MPTCP_PM_ADDR_ATTR_FAMILY] = { .type = NLA_U16, }, + [MPTCP_PM_ADDR_ATTR_ADDR4] = { .type = NLA_U32, }, + [MPTCP_PM_ADDR_ATTR_ADDR6] = { .len = 16, }, + [MPTCP_PM_ADDR_ATTR_ID] = { .type = NLA_U8, }, + [MPTCP_PM_ADDR_ATTR_PORT] = { .type = NLA_U16, }, + [MPTCP_PM_ADDR_ATTR_IF_IDX] = { .type = NLA_S32, }, + [MPTCP_PM_ADDR_ATTR_FLAGS] = { .type = NLA_U32, }, +}; + +/* MPTCP_PM_CMD_GET_ADDR - do */ +static const struct nla_policy mptcp_pm_get_addr_nl_policy[MPTCP_PM_ADDR_ATTR_IF_IDX + 1] = { + [MPTCP_PM_ADDR_ATTR_FAMILY] = { .type = NLA_U16, }, + [MPTCP_PM_ADDR_ATTR_ADDR4] = { .type = NLA_U32, }, + [MPTCP_PM_ADDR_ATTR_ADDR6] = { .len = 16, }, + [MPTCP_PM_ADDR_ATTR_ID] = { .type = NLA_U8, }, + [MPTCP_PM_ADDR_ATTR_PORT] = { .type = NLA_U16, }, + [MPTCP_PM_ADDR_ATTR_IF_IDX] = { .type = NLA_S32, }, + [MPTCP_PM_ADDR_ATTR_FLAGS] = { .type = NLA_U32, }, +}; + +/* MPTCP_PM_CMD_FLUSH_ADDRS - do */ +static const struct nla_policy mptcp_pm_flush_addrs_nl_policy[MPTCP_PM_ADDR_ATTR_IF_IDX + 1] = { + [MPTCP_PM_ADDR_ATTR_FAMILY] = { .type = NLA_U16, }, + [MPTCP_PM_ADDR_ATTR_ADDR4] = { .type = NLA_U32, }, + [MPTCP_PM_ADDR_ATTR_ADDR6] = { .len = 16, }, + [MPTCP_PM_ADDR_ATTR_ID] = { .type = NLA_U8, }, + [MPTCP_PM_ADDR_ATTR_PORT] = { .type = NLA_U16, }, + [MPTCP_PM_ADDR_ATTR_IF_IDX] = { .type = NLA_S32, }, + [MPTCP_PM_ADDR_ATTR_FLAGS] = { .type = NLA_U32, }, +}; + +/* MPTCP_PM_CMD_SET_LIMITS - do */ +static const struct nla_policy mptcp_pm_set_limits_nl_policy[MPTCP_PM_ATTR_SUBFLOWS + 1] = { + [MPTCP_PM_ATTR_RCV_ADD_ADDRS] = { .type = NLA_U32, }, + [MPTCP_PM_ATTR_SUBFLOWS] = { .type = NLA_U32, }, +}; + +/* MPTCP_PM_CMD_GET_LIMITS - do */ +static const struct nla_policy mptcp_pm_get_limits_nl_policy[MPTCP_PM_ATTR_SUBFLOWS + 1] = { + [MPTCP_PM_ATTR_RCV_ADD_ADDRS] = { .type = NLA_U32, }, + [MPTCP_PM_ATTR_SUBFLOWS] = { .type = NLA_U32, }, +}; + +/* MPTCP_PM_CMD_SET_FLAGS - do */ +static const struct nla_policy mptcp_pm_set_flags_nl_policy[MPTCP_PM_ADDR_ATTR_IF_IDX + 1] = { + [MPTCP_PM_ADDR_ATTR_FAMILY] = { .type = NLA_U16, }, + [MPTCP_PM_ADDR_ATTR_ADDR4] = { .type = NLA_U32, }, + [MPTCP_PM_ADDR_ATTR_ADDR6] = { .len = 16, }, + [MPTCP_PM_ADDR_ATTR_ID] = { .type = NLA_U8, }, + [MPTCP_PM_ADDR_ATTR_PORT] = { .type = NLA_U16, }, + [MPTCP_PM_ADDR_ATTR_IF_IDX] = { .type = NLA_S32, }, + [MPTCP_PM_ADDR_ATTR_FLAGS] = { .type = NLA_U32, }, +}; + +/* MPTCP_PM_CMD_ANNOUNCE - do */ +static const struct nla_policy mptcp_pm_announce_nl_policy[MPTCP_PM_ATTR_TOKEN + 1] = { + [MPTCP_PM_ATTR_TOKEN] = { .type = NLA_U32, }, + [MPTCP_PM_ATTR_ADDR] = NLA_POLICY_NESTED(mptcp_pm_endpoint_nl_policy), +}; + +/* MPTCP_PM_CMD_REMOVE - do */ +static const struct nla_policy mptcp_pm_remove_nl_policy[MPTCP_PM_ATTR_LOC_ID + 1] = { + [MPTCP_PM_ATTR_TOKEN] = { .type = NLA_U32, }, + [MPTCP_PM_ATTR_LOC_ID] = { .type = NLA_U8, }, +}; + +/* MPTCP_PM_CMD_SUBFLOW_CREATE - do */ +static const struct nla_policy mptcp_pm_subflow_create_nl_policy[MPTCP_PM_ATTR_ADDR_REMOTE + 1] = { + [MPTCP_PM_ATTR_ADDR_REMOTE] = NLA_POLICY_NESTED(mptcp_pm_endpoint_nl_policy), + [MPTCP_PM_ATTR_TOKEN] = { .type = NLA_U32, }, + [MPTCP_PM_ATTR_ADDR] = NLA_POLICY_NESTED(mptcp_pm_endpoint_nl_policy), +}; + +/* MPTCP_PM_CMD_SUBFLOW_DESTROY - do */ +static const struct nla_policy mptcp_pm_subflow_destroy_nl_policy[MPTCP_PM_ATTR_ADDR_REMOTE + 1] = { + [MPTCP_PM_ATTR_ADDR_REMOTE] = NLA_POLICY_NESTED(mptcp_pm_endpoint_nl_policy), + [MPTCP_PM_ATTR_TOKEN] = { .type = NLA_U32, }, + [MPTCP_PM_ATTR_ADDR] = NLA_POLICY_NESTED(mptcp_pm_endpoint_nl_policy), +}; + static struct pm_nl_pernet *pm_nl_get_pernet(const struct net *net) { return net_generic(net, pm_nl_pernet_id); @@ -1099,29 +1196,6 @@ static const struct genl_multicast_group mptcp_pm_mcgrps[] = { }, }; -static const struct nla_policy -mptcp_pm_addr_policy[MPTCP_PM_ADDR_ATTR_MAX + 1] = { - [MPTCP_PM_ADDR_ATTR_FAMILY] = { .type = NLA_U16, }, - [MPTCP_PM_ADDR_ATTR_ID] = { .type = NLA_U8, }, - [MPTCP_PM_ADDR_ATTR_ADDR4] = { .type = NLA_U32, }, - [MPTCP_PM_ADDR_ATTR_ADDR6] = - NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)), - [MPTCP_PM_ADDR_ATTR_PORT] = { .type = NLA_U16 }, - [MPTCP_PM_ADDR_ATTR_FLAGS] = { .type = NLA_U32 }, - [MPTCP_PM_ADDR_ATTR_IF_IDX] = { .type = NLA_S32 }, -}; - -static const struct nla_policy mptcp_pm_policy[MPTCP_PM_ATTR_MAX + 1] = { - [MPTCP_PM_ATTR_ADDR] = - NLA_POLICY_NESTED(mptcp_pm_addr_policy), - [MPTCP_PM_ATTR_RCV_ADD_ADDRS] = { .type = NLA_U32, }, - [MPTCP_PM_ATTR_SUBFLOWS] = { .type = NLA_U32, }, - [MPTCP_PM_ATTR_TOKEN] = { .type = NLA_U32, }, - [MPTCP_PM_ATTR_LOC_ID] = { .type = NLA_U8, }, - [MPTCP_PM_ATTR_ADDR_REMOTE] = - NLA_POLICY_NESTED(mptcp_pm_addr_policy), -}; - void mptcp_pm_nl_subflow_chk_stale(const struct mptcp_sock *msk, struct sock *ssk) { struct mptcp_subflow_context *iter, *subflow = mptcp_subflow_ctx(ssk); @@ -1171,6 +1245,7 @@ static int mptcp_pm_family_to_addr(int family) static int mptcp_pm_parse_pm_addr_attr(struct nlattr *tb[], const struct nlattr *attr, struct genl_info *info, + const struct nla_policy *nla_policy, struct mptcp_addr_info *addr, bool require_family) { @@ -1183,7 +1258,8 @@ static int mptcp_pm_parse_pm_addr_attr(struct nlattr *tb[], /* no validation needed - was already done via nested policy */ err = nla_parse_nested_deprecated(tb, MPTCP_PM_ADDR_ATTR_MAX, attr, - mptcp_pm_addr_policy, info->extack); + nla_policy, + info->extack); if (err) return err; @@ -1236,19 +1312,22 @@ int mptcp_pm_parse_addr(struct nlattr *attr, struct genl_info *info, memset(addr, 0, sizeof(*addr)); - return mptcp_pm_parse_pm_addr_attr(tb, attr, info, addr, true); + return mptcp_pm_parse_pm_addr_attr(tb, attr, info, mptcp_pm_endpoint_nl_policy, addr, true); } int mptcp_pm_parse_entry(struct nlattr *attr, struct genl_info *info, - bool require_family, + int cmd, bool require_family, struct mptcp_pm_addr_entry *entry) { struct nlattr *tb[MPTCP_PM_ADDR_ATTR_MAX + 1]; + const struct nla_policy *nla_policy = mptcp_nl_cmd2policy(cmd); int err; memset(entry, 0, sizeof(*entry)); - err = mptcp_pm_parse_pm_addr_attr(tb, attr, info, &entry->addr, require_family); + err = mptcp_pm_parse_pm_addr_attr(tb, attr, info, + nla_policy, + &entry->addr, require_family); if (err) return err; @@ -1305,7 +1384,7 @@ static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info) struct mptcp_pm_addr_entry addr, *entry; int ret; - ret = mptcp_pm_parse_entry(attr, info, true, &addr); + ret = mptcp_pm_parse_entry(attr, info, MPTCP_PM_CMD_ADD_ADDR, true, &addr); if (ret < 0) return ret; @@ -1487,7 +1566,7 @@ static int mptcp_nl_cmd_del_addr(struct sk_buff *skb, struct genl_info *info) unsigned int addr_max; int ret; - ret = mptcp_pm_parse_entry(attr, info, false, &addr); + ret = mptcp_pm_parse_entry(attr, info, MPTCP_PM_CMD_DEL_ADDR, false, &addr); if (ret < 0) return ret; @@ -1679,7 +1758,7 @@ static int mptcp_nl_cmd_get_addr(struct sk_buff *skb, struct genl_info *info) void *reply; int ret; - ret = mptcp_pm_parse_entry(attr, info, false, &addr); + ret = mptcp_pm_parse_entry(attr, info, MPTCP_PM_CMD_GET_ADDR, false, &addr); if (ret < 0) return ret; @@ -1925,12 +2004,12 @@ static int mptcp_nl_cmd_set_flags(struct sk_buff *skb, struct genl_info *info) u8 bkup = 0; int ret; - ret = mptcp_pm_parse_entry(attr, info, false, &addr); + ret = mptcp_pm_parse_entry(attr, info, MPTCP_PM_CMD_SET_FLAGS, false, &addr); if (ret < 0) return ret; if (attr_rem) { - ret = mptcp_pm_parse_entry(attr_rem, info, false, &remote); + ret = mptcp_pm_parse_entry(attr_rem, info, MPTCP_PM_CMD_SET_FLAGS, false, &remote); if (ret < 0) return ret; } @@ -2278,72 +2357,116 @@ void mptcp_event(enum mptcp_event_type type, const struct mptcp_sock *msk, nlmsg_free(skb); } -static const struct genl_small_ops mptcp_pm_ops[] = { + +const struct genl_ops mptcp_pm_ops[] = { { - .cmd = MPTCP_PM_CMD_ADD_ADDR, - .doit = mptcp_nl_cmd_add_addr, - .flags = GENL_UNS_ADMIN_PERM, + .cmd = MPTCP_PM_CMD_ADD_ADDR, + .validate = GENL_DONT_VALIDATE_STRICT, + .doit = mptcp_nl_cmd_add_addr, + .policy = mptcp_pm_add_addr_nl_policy, + .maxattr = MPTCP_PM_ADDR_ATTR_IF_IDX, + .flags = GENL_UNS_ADMIN_PERM, }, { - .cmd = MPTCP_PM_CMD_DEL_ADDR, - .doit = mptcp_nl_cmd_del_addr, - .flags = GENL_UNS_ADMIN_PERM, + .cmd = MPTCP_PM_CMD_DEL_ADDR, + .validate = GENL_DONT_VALIDATE_STRICT, + .doit = mptcp_nl_cmd_del_addr, + .policy = mptcp_pm_del_addr_nl_policy, + .maxattr = MPTCP_PM_ADDR_ATTR_IF_IDX, + .flags = GENL_UNS_ADMIN_PERM, }, { - .cmd = MPTCP_PM_CMD_FLUSH_ADDRS, - .doit = mptcp_nl_cmd_flush_addrs, - .flags = GENL_UNS_ADMIN_PERM, + .cmd = MPTCP_PM_CMD_GET_ADDR, + .validate = GENL_DONT_VALIDATE_STRICT, + .doit = mptcp_nl_cmd_get_addr, + .dumpit = mptcp_nl_cmd_dump_addrs, + .policy = mptcp_pm_get_addr_nl_policy, + .maxattr = MPTCP_PM_ADDR_ATTR_IF_IDX, + .flags = GENL_UNS_ADMIN_PERM, }, { - .cmd = MPTCP_PM_CMD_GET_ADDR, - .doit = mptcp_nl_cmd_get_addr, - .dumpit = mptcp_nl_cmd_dump_addrs, + .cmd = MPTCP_PM_CMD_FLUSH_ADDRS, + .validate = GENL_DONT_VALIDATE_STRICT, + .doit = mptcp_nl_cmd_flush_addrs, + .policy = mptcp_pm_flush_addrs_nl_policy, + .maxattr = MPTCP_PM_ADDR_ATTR_IF_IDX, + .flags = GENL_UNS_ADMIN_PERM, }, { - .cmd = MPTCP_PM_CMD_SET_LIMITS, - .doit = mptcp_nl_cmd_set_limits, - .flags = GENL_UNS_ADMIN_PERM, + .cmd = MPTCP_PM_CMD_SET_LIMITS, + .validate = GENL_DONT_VALIDATE_STRICT, + .doit = mptcp_nl_cmd_set_limits, + .policy = mptcp_pm_set_limits_nl_policy, + .maxattr = MPTCP_PM_ATTR_SUBFLOWS, + .flags = GENL_UNS_ADMIN_PERM, }, { - .cmd = MPTCP_PM_CMD_GET_LIMITS, - .doit = mptcp_nl_cmd_get_limits, + .cmd = MPTCP_PM_CMD_GET_LIMITS, + .validate = GENL_DONT_VALIDATE_STRICT, + .doit = mptcp_nl_cmd_get_limits, + .policy = mptcp_pm_get_limits_nl_policy, + .maxattr = MPTCP_PM_ATTR_SUBFLOWS, }, { - .cmd = MPTCP_PM_CMD_SET_FLAGS, - .doit = mptcp_nl_cmd_set_flags, - .flags = GENL_UNS_ADMIN_PERM, + .cmd = MPTCP_PM_CMD_SET_FLAGS, + .validate = GENL_DONT_VALIDATE_STRICT, + .doit = mptcp_nl_cmd_set_flags, + .policy = mptcp_pm_set_flags_nl_policy, + .maxattr = MPTCP_PM_ADDR_ATTR_IF_IDX, + .flags = GENL_UNS_ADMIN_PERM, }, { - .cmd = MPTCP_PM_CMD_ANNOUNCE, - .doit = mptcp_nl_cmd_announce, - .flags = GENL_UNS_ADMIN_PERM, + .cmd = MPTCP_PM_CMD_ANNOUNCE, + .validate = GENL_DONT_VALIDATE_STRICT, + .doit = mptcp_nl_cmd_announce, + .policy = mptcp_pm_announce_nl_policy, + .maxattr = MPTCP_PM_ATTR_TOKEN, + .flags = GENL_UNS_ADMIN_PERM, }, { - .cmd = MPTCP_PM_CMD_REMOVE, - .doit = mptcp_nl_cmd_remove, - .flags = GENL_UNS_ADMIN_PERM, + .cmd = MPTCP_PM_CMD_REMOVE, + .validate = GENL_DONT_VALIDATE_STRICT, + .doit = mptcp_nl_cmd_remove, + .policy = mptcp_pm_remove_nl_policy, + .maxattr = MPTCP_PM_ATTR_LOC_ID, + .flags = GENL_UNS_ADMIN_PERM, }, { - .cmd = MPTCP_PM_CMD_SUBFLOW_CREATE, - .doit = mptcp_nl_cmd_sf_create, - .flags = GENL_UNS_ADMIN_PERM, + .cmd = MPTCP_PM_CMD_SUBFLOW_CREATE, + .validate = GENL_DONT_VALIDATE_STRICT, + .doit = mptcp_nl_cmd_sf_create, + .policy = mptcp_pm_subflow_create_nl_policy, + .maxattr = MPTCP_PM_ATTR_ADDR_REMOTE, + .flags = GENL_UNS_ADMIN_PERM, }, { - .cmd = MPTCP_PM_CMD_SUBFLOW_DESTROY, - .doit = mptcp_nl_cmd_sf_destroy, - .flags = GENL_UNS_ADMIN_PERM, + .cmd = MPTCP_PM_CMD_SUBFLOW_DESTROY, + .validate = GENL_DONT_VALIDATE_STRICT, + .doit = mptcp_nl_cmd_sf_destroy, + .policy = mptcp_pm_subflow_destroy_nl_policy, + .maxattr = MPTCP_PM_ATTR_ADDR_REMOTE, + .flags = GENL_UNS_ADMIN_PERM, }, }; +static const struct nla_policy *mptcp_nl_cmd2policy(int cmd) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(mptcp_pm_ops); i++) + if (mptcp_pm_ops[i].cmd == cmd) + return mptcp_pm_ops[i].policy; + return NULL; +} + static struct genl_family mptcp_genl_family __ro_after_init = { .name = MPTCP_PM_NAME, .version = MPTCP_PM_VER, .maxattr = MPTCP_PM_ATTR_MAX, - .policy = mptcp_pm_policy, .netnsok = true, .module = THIS_MODULE, - .small_ops = mptcp_pm_ops, - .n_small_ops = ARRAY_SIZE(mptcp_pm_ops), + .ops = mptcp_pm_ops, + .n_ops = ARRAY_SIZE(mptcp_pm_ops), .resv_start_op = MPTCP_PM_CMD_SUBFLOW_DESTROY + 1, .mcgrps = mptcp_pm_mcgrps, .n_mcgrps = ARRAY_SIZE(mptcp_pm_mcgrps), diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c index 47a883a16c11..7e30213eaaf1 100644 --- a/net/mptcp/pm_userspace.c +++ b/net/mptcp/pm_userspace.c @@ -172,7 +172,7 @@ int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info) goto announce_err; } - err = mptcp_pm_parse_entry(addr, info, true, &addr_val); + err = mptcp_pm_parse_entry(addr, info, MPTCP_PM_CMD_ADD_ADDR, true, &addr_val); if (err < 0) { GENL_SET_ERR_MSG(info, "error parsing local address"); goto announce_err; diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index d2e59cf33f57..e45e4a9e73c8 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -803,7 +803,7 @@ void mptcp_pm_data_reset(struct mptcp_sock *msk); int mptcp_pm_parse_addr(struct nlattr *attr, struct genl_info *info, struct mptcp_addr_info *addr); int mptcp_pm_parse_entry(struct nlattr *attr, struct genl_info *info, - bool require_family, + int cmd, bool require_family, struct mptcp_pm_addr_entry *entry); bool mptcp_pm_addr_families_match(const struct sock *sk, const struct mptcp_addr_info *loc,
this prepares MPTCP control plane to be described as YAML spec. Link: https://github.com/multipath-tcp/mptcp_net-next/issues/340 Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- net/mptcp/pm_netlink.c | 259 +++++++++++++++++++++++++++++---------- net/mptcp/pm_userspace.c | 2 +- net/mptcp/protocol.h | 2 +- 3 files changed, 193 insertions(+), 70 deletions(-)