[v1] mptcp: allow changing the "backup" bit by endpoint id

[PATCH mptcp-next] mptcp: allow changing the "backup" bit by endpoint id

Posted by Davide Caratti 2 years, 5 months ago

a non-zero 'id' is sufficient to identify MPTCP endpoints: allow changing
the value of 'backup' bit by simply specifying the endpoint id.

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/158
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 net/mptcp/pm_netlink.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 7b96be1e9f14..b4c647e67930 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1700,24 +1700,35 @@ static int mptcp_nl_addr_backup(struct net *net,
 	return ret;
 }
 
+static int mptcp_nl_set_flags_idzero(u8 flags)
+{
+	return -EOPNOTSUPP;
+}
+
 static int mptcp_nl_cmd_set_flags(struct sk_buff *skb, struct genl_info *info)
 {
+	struct mptcp_pm_addr_entry addr = { .addr = { .family = AF_UNSPEC }, }, *entry;
 	struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
 	struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
-	struct mptcp_pm_addr_entry addr, *entry;
 	struct net *net = sock_net(skb->sk);
-	u8 bkup = 0;
+	u8 bkup = 0, lookup_by_id = 0;
 	int ret;
 
-	ret = mptcp_pm_parse_addr(attr, info, true, &addr);
+	ret = mptcp_pm_parse_addr(attr, info, false, &addr);
 	if (ret < 0)
 		return ret;
 
 	if (addr.flags & MPTCP_PM_ADDR_FLAG_BACKUP)
 		bkup = 1;
+	if (addr.addr.family == AF_UNSPEC) {
+		lookup_by_id = 1;
+		if (!addr.addr.id)
+			return mptcp_nl_set_flags_idzero(addr.flags);
+	}
 
 	list_for_each_entry(entry, &pernet->local_addr_list, list) {
-		if (addresses_equal(&entry->addr, &addr.addr, true)) {
+		if ((!lookup_by_id && addresses_equal(&entry->addr, &addr.addr, true)) ||
+		    (lookup_by_id && entry->addr.id == addr.addr.id)) {
 			mptcp_nl_addr_backup(net, &entry->addr, bkup);
 
 			if (bkup)
-- 
2.31.1

Re: [PATCH mptcp-next] mptcp: allow changing the "backup" bit by endpoint id

Posted by Mat Martineau 2 years, 5 months ago

On Thu, 11 Nov 2021, Davide Caratti wrote:

> a non-zero 'id' is sufficient to identify MPTCP endpoints: allow changing
> the value of 'backup' bit by simply specifying the endpoint id.
>
> Link: https://github.com/multipath-tcp/mptcp_net-next/issues/158
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
> net/mptcp/pm_netlink.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 7b96be1e9f14..b4c647e67930 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1700,24 +1700,35 @@ static int mptcp_nl_addr_backup(struct net *net,
> 	return ret;
> }
>
> +static int mptcp_nl_set_flags_idzero(u8 flags)
> +{
> +	return -EOPNOTSUPP;
> +}

Hi Davide -

Is the above function added as a placeholder for something in the future? 
Seems like a direct return below would be clearer.

- Mat


> +
> static int mptcp_nl_cmd_set_flags(struct sk_buff *skb, struct genl_info *info)
> {
> +	struct mptcp_pm_addr_entry addr = { .addr = { .family = AF_UNSPEC }, }, *entry;
> 	struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
> 	struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
> -	struct mptcp_pm_addr_entry addr, *entry;
> 	struct net *net = sock_net(skb->sk);
> -	u8 bkup = 0;
> +	u8 bkup = 0, lookup_by_id = 0;
> 	int ret;
>
> -	ret = mptcp_pm_parse_addr(attr, info, true, &addr);
> +	ret = mptcp_pm_parse_addr(attr, info, false, &addr);
> 	if (ret < 0)
> 		return ret;
>
> 	if (addr.flags & MPTCP_PM_ADDR_FLAG_BACKUP)
> 		bkup = 1;
> +	if (addr.addr.family == AF_UNSPEC) {
> +		lookup_by_id = 1;
> +		if (!addr.addr.id)
> +			return mptcp_nl_set_flags_idzero(addr.flags);
> +	}
>
> 	list_for_each_entry(entry, &pernet->local_addr_list, list) {
> -		if (addresses_equal(&entry->addr, &addr.addr, true)) {
> +		if ((!lookup_by_id && addresses_equal(&entry->addr, &addr.addr, true)) ||
> +		    (lookup_by_id && entry->addr.id == addr.addr.id)) {
> 			mptcp_nl_addr_backup(net, &entry->addr, bkup);
>
> 			if (bkup)
> -- 
> 2.31.1
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next] mptcp: allow changing the "backup" bit by endpoint id

Posted by Davide Caratti 2 years, 5 months ago

hello Mat, thanks for reading,

On Thu, Nov 11, 2021 at 04:09:13PM -0800, Mat Martineau wrote:
> On Thu, 11 Nov 2021, Davide Caratti wrote:
> 
> > a non-zero 'id' is sufficient to identify MPTCP endpoints: allow changing
> > the value of 'backup' bit by simply specifying the endpoint id.
> > 
> > Link: https://github.com/multipath-tcp/mptcp_net-next/issues/158
> > Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> > ---
> > net/mptcp/pm_netlink.c | 19 +++++++++++++++----
> > 1 file changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index 7b96be1e9f14..b4c647e67930 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -1700,24 +1700,35 @@ static int mptcp_nl_addr_backup(struct net *net,
> > 	return ret;
> > }
> > 
> > +static int mptcp_nl_set_flags_idzero(u8 flags)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> 
> Hi Davide -
> 
> Is the above function added as a placeholder for something in the future?

yes it's meant to be a placeholder, and I should have mentioned it in the
commit message. Any advice on what should happen from the functional
point of view when we change the "backup" bit for id=0 ?

> Seems like a direct return below would be clearer.

with a TODO comment, also. But just aesthetic, because probably the
compiler should make no difference and inline the call to
mptcp_nl_set_flags_idzero().

-- 
davide

Re: [PATCH mptcp-next] mptcp: allow changing the "backup" bit by endpoint id

Posted by Paolo Abeni 2 years, 5 months ago

On Fri, 2021-11-12 at 16:06 +0100, Davide Caratti wrote:
> hello Mat, thanks for reading,
> 
> On Thu, Nov 11, 2021 at 04:09:13PM -0800, Mat Martineau wrote:
> > On Thu, 11 Nov 2021, Davide Caratti wrote:
> > 
> > > a non-zero 'id' is sufficient to identify MPTCP endpoints: allow changing
> > > the value of 'backup' bit by simply specifying the endpoint id.
> > > 
> > > Link: https://github.com/multipath-tcp/mptcp_net-next/issues/158
> > > Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> > > ---
> > > net/mptcp/pm_netlink.c | 19 +++++++++++++++----
> > > 1 file changed, 15 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > > index 7b96be1e9f14..b4c647e67930 100644
> > > --- a/net/mptcp/pm_netlink.c
> > > +++ b/net/mptcp/pm_netlink.c
> > > @@ -1700,24 +1700,35 @@ static int mptcp_nl_addr_backup(struct net *net,
> > > 	return ret;
> > > }
> > > 
> > > +static int mptcp_nl_set_flags_idzero(u8 flags)
> > > +{
> > > +	return -EOPNOTSUPP;
> > > +}
> > 
> > Hi Davide -
> > 
> > Is the above function added as a placeholder for something in the future?
> 
> yes it's meant to be a placeholder, and I should have mentioned it in the
> commit message. Any advice on what should happen from the functional
> point of view when we change the "backup" bit for id=0 ?

IMHO explicitly forbidding the switch of the backup flag on id 0 is the
correct thing to do. I guess/expect the backup flag will change in
response to some "meta-change" on the related link.

e.g. the end-user has free 5g connectivity when attached to the home's
cell, and turn off backup flag on the address[es] attaches to the 5G
link.

I don't easily see an use-case for id 0

BTW:

> +	struct mptcp_pm_addr_entry addr = { .addr = { .family = AF_UNSPEC }, }, *entry; 

Is checkpatch happy about the above? I would move '*entry' on a
different line for readability's sake

> > Seems like a direct return below would be clearer.
> 
> with a TODO comment, also. But just aesthetic, because probably the
> compiler should make no difference and inline the call to
> mptcp_nl_set_flags_idzero().

Yep, generated-code wise would be the same. If we choose to forbid the
id 0, I think it will made the code more readable.

Cheers,

Paolo