[PATCH mptcp-next] mptcp: use nla_parse_nested

[PATCH mptcp-next] mptcp: use nla_parse_nested

[Geliang Tang]

NLA_F_NESTED has been used in both 'pm_nl_ctl' and 'ip mptcp'. So we can
use the strict mode parsing function nla_parse_nested() instead of the
deprecated one nla_parse_nested_deprecated().

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_netlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 93800f32fcb6..dbf1041876f5 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1114,8 +1114,8 @@ static int mptcp_pm_parse_addr(struct nlattr *attr, struct genl_info *info,
 	}
 
 	/* 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);
+	err = nla_parse_nested(tb, MPTCP_PM_ADDR_ATTR_MAX, attr,
+			       mptcp_pm_addr_policy, info->extack);
 	if (err)
 		return err;
 
-- 
2.31.1

Re: mptcp: use nla_parse_nested: Build Failure

[MPTCP CI]

Hi Geliang,

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/022caed0c9e5964f525336aced8233338c29386f.1643437533.git.geliang.tang@suse.com/
  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/1765070423

Status: failure
Initiator: MPTCPimporter
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/8e73ec0f9b70

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

Re: mptcp: use nla_parse_nested: Build Failure

[Matthieu Baerts]

Hello,

On 29/01/2022 08:18, MPTCP CI wrote:
> Hi Geliang,
> 
> 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/022caed0c9e5964f525336aced8233338c29386f.1643437533.git.geliang.tang@suse.com/
>   https://github.com/multipath-tcp/mptcp_net-next/actions/runs/1765070423

The issue is not in the code but due to patchwork. I don't see this
patch there, nor on lore.

You can ignore this.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next] mptcp: use nla_parse_nested

[Matthieu Baerts]

Hi Geliang,

On 29/01/2022 07:26, Geliang Tang wrote:
> NLA_F_NESTED has been used in both 'pm_nl_ctl' and 'ip mptcp'.

Was it always the case for 'ip mptcp'? I mean, we cannot break the user API.

And even if it was always the case for 'ip mptcp', there could be other
tools not using NLA_F_NESTED. Maybe we can ignore them in this specific
case but I'm not sure.

Cheers,
Matt

> So we can
> use the strict mode parsing function nla_parse_nested() instead of the
> deprecated one nla_parse_nested_deprecated().

-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next] mptcp: use nla_parse_nested

[Geliang Tang]

Hi Matt,

Thanks for your review.

I don't know why this commit didn't show in the patchwork. The CI seems to
have complained about it.

On Sat, Jan 29, 2022 at 12:53:20PM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 29/01/2022 07:26, Geliang Tang wrote:
> > NLA_F_NESTED has been used in both 'pm_nl_ctl' and 'ip mptcp'.
> 
> Was it always the case for 'ip mptcp'? I mean, we cannot break the user API.
> 
> And even if it was always the case for 'ip mptcp', there could be other
> tools not using NLA_F_NESTED. Maybe we can ignore them in this specific
> case but I'm not sure.

I think now is the right time to switch to the new API: nla_parse_nested.
Since now no other tool uses our mptcp netlink yet, only 'ip mptcp' and
'pm_nl_ctl'. There's no need to consider about the compatibility. The old
API - nla_parse_nested_deprecated - is reserved for the compatibility of
the netlinks that already have some user tools using its interface in the
non-NLA_F_NESTED way. We are not the case.

Switch to the new API can force the user tools to use the NLA_F_NESTED way
from now on. That makes sure no user tool uses the non-NLA_F_NESTED way for
our mptcp netlink. Otherwise, one day the old API will be abandoned, and
then if there're some user tools use the non-NLA_F_NESTED way, we will have
to deal with the compatibility at that time. If we switch to the new API
now, we can avoid this potential trouble.

This is only my opinion, and I'm not very sure either. :)

Thanks,
-Geliang

> 
> Cheers,
> Matt
> 
> > So we can
> > use the strict mode parsing function nla_parse_nested() instead of the
> > deprecated one nla_parse_nested_deprecated().
> 
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
> 

Re: [PATCH mptcp-next] mptcp: use nla_parse_nested

[Matthieu Baerts]

Hi Geliang,

On 30/01/2022 05:33, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for your review.
> 
> I don't know why this commit didn't show in the patchwork. The CI seems to
> have complained about it.

It seems the issue was on kernel.org side, now fixed: your patch is on
patchwork and lore.

> On Sat, Jan 29, 2022 at 12:53:20PM +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 29/01/2022 07:26, Geliang Tang wrote:
>>> NLA_F_NESTED has been used in both 'pm_nl_ctl' and 'ip mptcp'.
>>
>> Was it always the case for 'ip mptcp'? I mean, we cannot break the user API.
>>
>> And even if it was always the case for 'ip mptcp', there could be other
>> tools not using NLA_F_NESTED. Maybe we can ignore them in this specific
>> case but I'm not sure.
> 
> I think now is the right time to switch to the new API: nla_parse_nested.
> Since now no other tool uses our mptcp netlink yet, only 'ip mptcp' and
> 'pm_nl_ctl'. There's no need to consider about the compatibility.

I would like to only consider Open-Source tools but I don't think we
can. It is unlikely someone wrote a new tool not using NLA_F_NESTED but
I don't think we can be 100% sure. This is an uAPI break to change this
I think, no?

> The old
> API - nla_parse_nested_deprecated - is reserved for the compatibility of
> the netlinks that already have some user tools using its interface in the
> non-NLA_F_NESTED way. We are not the case.
> 
> Switch to the new API can force the user tools to use the NLA_F_NESTED way
> from now on. That makes sure no user tool uses the non-NLA_F_NESTED way for
> our mptcp netlink. Otherwise, one day the old API will be abandoned, and
> then if there're some user tools use the non-NLA_F_NESTED way, we will have
> to deal with the compatibility at that time. If we switch to the new API
> now, we can avoid this potential trouble.

I guess the compatibility will need to be kept of a bit of time. This
function is used in 250+ places in the kernel. I guess it has the
"deprecated" word in the name to clearly indicate "you should not use
that for new features" but not to say "in a few months, we will drop
this, deal with it now.", no?

It would be easier to use nla_parse_nested() instead of
nla_parse_nested_deprecated() but I don't think we can.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next] mptcp: use nla_parse_nested

[Mat Martineau]

On Mon, 31 Jan 2022, Matthieu Baerts wrote:

> Hi Geliang,
>
> On 30/01/2022 05:33, Geliang Tang wrote:
>> Hi Matt,
>>
>> Thanks for your review.
>>
>> I don't know why this commit didn't show in the patchwork. The CI seems to
>> have complained about it.
>
> It seems the issue was on kernel.org side, now fixed: your patch is on
> patchwork and lore.
>
>> On Sat, Jan 29, 2022 at 12:53:20PM +0100, Matthieu Baerts wrote:
>>> Hi Geliang,
>>>
>>> On 29/01/2022 07:26, Geliang Tang wrote:
>>>> NLA_F_NESTED has been used in both 'pm_nl_ctl' and 'ip mptcp'.
>>>
>>> Was it always the case for 'ip mptcp'? I mean, we cannot break the user API.
>>>
>>> And even if it was always the case for 'ip mptcp', there could be other
>>> tools not using NLA_F_NESTED. Maybe we can ignore them in this specific
>>> case but I'm not sure.
>>
>> I think now is the right time to switch to the new API: nla_parse_nested.
>> Since now no other tool uses our mptcp netlink yet, only 'ip mptcp' and
>> 'pm_nl_ctl'. There's no need to consider about the compatibility.
>
> I would like to only consider Open-Source tools but I don't think we
> can. It is unlikely someone wrote a new tool not using NLA_F_NESTED but
> I don't think we can be 100% sure. This is an uAPI break to change this
> I think, no?
>
>> The old
>> API - nla_parse_nested_deprecated - is reserved for the compatibility of
>> the netlinks that already have some user tools using its interface in the
>> non-NLA_F_NESTED way. We are not the case.
>>
>> Switch to the new API can force the user tools to use the NLA_F_NESTED way
>> from now on. That makes sure no user tool uses the non-NLA_F_NESTED way for
>> our mptcp netlink. Otherwise, one day the old API will be abandoned, and
>> then if there're some user tools use the non-NLA_F_NESTED way, we will have
>> to deal with the compatibility at that time. If we switch to the new API
>> now, we can avoid this potential trouble.
>
> I guess the compatibility will need to be kept of a bit of time. This
> function is used in 250+ places in the kernel. I guess it has the
> "deprecated" word in the name to clearly indicate "you should not use
> that for new features" but not to say "in a few months, we will drop
> this, deal with it now.", no?
>

It seems like the meaning of "deprecated" is the former ("don't add new 
calls to this")... but we did add our use of the function in 2020 after it 
was already deprecated! Given the "do not break userspace" rule for kernel 
changes, I don't expect the deprecated to go away and for now I think 
we should leave it unchanged. When we discuss patches at this week's 
meeting I'll see if others agree.

> It would be easier to use nla_parse_nested() instead of
> nla_parse_nested_deprecated() but I don't think we can.
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next] mptcp: use nla_parse_nested

[Paolo Abeni]

On Wed, 2022-02-02 at 16:38 -0800, Mat Martineau wrote:
> On Mon, 31 Jan 2022, Matthieu Baerts wrote:
> 
> > Hi Geliang,
> > 
> > On 30/01/2022 05:33, Geliang Tang wrote:
> > > Hi Matt,
> > > 
> > > Thanks for your review.
> > > 
> > > I don't know why this commit didn't show in the patchwork. The CI seems to
> > > have complained about it.
> > 
> > It seems the issue was on kernel.org side, now fixed: your patch is on
> > patchwork and lore.
> > 
> > > On Sat, Jan 29, 2022 at 12:53:20PM +0100, Matthieu Baerts wrote:
> > > > Hi Geliang,
> > > > 
> > > > On 29/01/2022 07:26, Geliang Tang wrote:
> > > > > NLA_F_NESTED has been used in both 'pm_nl_ctl' and 'ip mptcp'.
> > > > 
> > > > Was it always the case for 'ip mptcp'? I mean, we cannot break the user API.
> > > > 
> > > > And even if it was always the case for 'ip mptcp', there could be other
> > > > tools not using NLA_F_NESTED. Maybe we can ignore them in this specific
> > > > case but I'm not sure.
> > > 
> > > I think now is the right time to switch to the new API: nla_parse_nested.
> > > Since now no other tool uses our mptcp netlink yet, only 'ip mptcp' and
> > > 'pm_nl_ctl'. There's no need to consider about the compatibility.
> > 
> > I would like to only consider Open-Source tools but I don't think we
> > can. It is unlikely someone wrote a new tool not using NLA_F_NESTED but
> > I don't think we can be 100% sure. This is an uAPI break to change this
> > I think, no?
> > 
> > > The old
> > > API - nla_parse_nested_deprecated - is reserved for the compatibility of
> > > the netlinks that already have some user tools using its interface in the
> > > non-NLA_F_NESTED way. We are not the case.
> > > 
> > > Switch to the new API can force the user tools to use the NLA_F_NESTED way
> > > from now on. That makes sure no user tool uses the non-NLA_F_NESTED way for
> > > our mptcp netlink. Otherwise, one day the old API will be abandoned, and
> > > then if there're some user tools use the non-NLA_F_NESTED way, we will have
> > > to deal with the compatibility at that time. If we switch to the new API
> > > now, we can avoid this potential trouble.
> > 
> > I guess the compatibility will need to be kept of a bit of time. This
> > function is used in 250+ places in the kernel. I guess it has the
> > "deprecated" word in the name to clearly indicate "you should not use
> > that for new features" but not to say "in a few months, we will drop
> > this, deal with it now.", no?
> > 
> 
> It seems like the meaning of "deprecated" is the former ("don't add new 
> calls to this")... but we did add our use of the function in 2020 after it 
> was already deprecated! Given the "do not break userspace" rule for kernel 
> changes, I don't expect the deprecated to go away and for now I think 
> we should leave it unchanged. When we discuss patches at this week's 
> meeting I'll see if others agree.

All known users (self-tests and iproute2) have NLA_F_NESTED since the
first implementation and I doubt there are other user-space users out
there.
 
Still the downside of breaking user-space are quite bad, and there are
other in kernel users of the deprecated api added even later then
MPTCP.

I think we can keep the existing code.

Cheers,

Paolo

Re: [PATCH mptcp-next] mptcp: use nla_parse_nested

[Geliang Tang]

On Fri, Feb 04, 2022 at 11:16:21AM +0100, Paolo Abeni wrote:
> On Wed, 2022-02-02 at 16:38 -0800, Mat Martineau wrote:
> > On Mon, 31 Jan 2022, Matthieu Baerts wrote:
> > 
> > > Hi Geliang,
> > > 
> > > On 30/01/2022 05:33, Geliang Tang wrote:
> > > > Hi Matt,
> > > > 
> > > > Thanks for your review.
> > > > 
> > > > I don't know why this commit didn't show in the patchwork. The CI seems to
> > > > have complained about it.
> > > 
> > > It seems the issue was on kernel.org side, now fixed: your patch is on
> > > patchwork and lore.
> > > 
> > > > On Sat, Jan 29, 2022 at 12:53:20PM +0100, Matthieu Baerts wrote:
> > > > > Hi Geliang,
> > > > > 
> > > > > On 29/01/2022 07:26, Geliang Tang wrote:
> > > > > > NLA_F_NESTED has been used in both 'pm_nl_ctl' and 'ip mptcp'.
> > > > > 
> > > > > Was it always the case for 'ip mptcp'? I mean, we cannot break the user API.
> > > > > 
> > > > > And even if it was always the case for 'ip mptcp', there could be other
> > > > > tools not using NLA_F_NESTED. Maybe we can ignore them in this specific
> > > > > case but I'm not sure.
> > > > 
> > > > I think now is the right time to switch to the new API: nla_parse_nested.
> > > > Since now no other tool uses our mptcp netlink yet, only 'ip mptcp' and
> > > > 'pm_nl_ctl'. There's no need to consider about the compatibility.
> > > 
> > > I would like to only consider Open-Source tools but I don't think we
> > > can. It is unlikely someone wrote a new tool not using NLA_F_NESTED but
> > > I don't think we can be 100% sure. This is an uAPI break to change this
> > > I think, no?
> > > 
> > > > The old
> > > > API - nla_parse_nested_deprecated - is reserved for the compatibility of
> > > > the netlinks that already have some user tools using its interface in the
> > > > non-NLA_F_NESTED way. We are not the case.
> > > > 
> > > > Switch to the new API can force the user tools to use the NLA_F_NESTED way
> > > > from now on. That makes sure no user tool uses the non-NLA_F_NESTED way for
> > > > our mptcp netlink. Otherwise, one day the old API will be abandoned, and
> > > > then if there're some user tools use the non-NLA_F_NESTED way, we will have
> > > > to deal with the compatibility at that time. If we switch to the new API
> > > > now, we can avoid this potential trouble.
> > > 
> > > I guess the compatibility will need to be kept of a bit of time. This
> > > function is used in 250+ places in the kernel. I guess it has the
> > > "deprecated" word in the name to clearly indicate "you should not use
> > > that for new features" but not to say "in a few months, we will drop
> > > this, deal with it now.", no?
> > > 
> > 
> > It seems like the meaning of "deprecated" is the former ("don't add new 
> > calls to this")... but we did add our use of the function in 2020 after it 
> > was already deprecated! Given the "do not break userspace" rule for kernel 
> > changes, I don't expect the deprecated to go away and for now I think 
> > we should leave it unchanged. When we discuss patches at this week's 
> > meeting I'll see if others agree.
> 
> All known users (self-tests and iproute2) have NLA_F_NESTED since the
> first implementation and I doubt there are other user-space users out
> there.
>  
> Still the downside of breaking user-space are quite bad, and there are
> other in kernel users of the deprecated api added even later then
> MPTCP.
> 
> I think we can keep the existing code.

Agree.

Thanks,
-Geliang

> 
> Cheers,
> 
> Paolo
>