[PATCH net] diag: fix netlink API attributes

[PATCH net] diag: fix netlink API attributes

[Nicolas Dichtel]

The first netlink attribute (value 0) must always be defined as none/unspec.
This is correctly done in inet_diag.h, but other diag interfaces are broken.

Libraries like libnl skip this kind of attributes, thus it's never reported to
the application.

CC: Thomas Graf <tgraf@suug.ch>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---

Ok, I know that this patch change the API, but it's really painful to have such
broken API, is it really mandatory to live with this?
And the question is: how to avoid to have the same mistake in the next diag API?

 include/uapi/linux/netlink_diag.h | 1 +
 include/uapi/linux/packet_diag.h  | 1 +
 include/uapi/linux/unix_diag.h    | 1 +
 3 files changed, 3 insertions(+)

diff --git a/include/uapi/linux/netlink_diag.h b/include/uapi/linux/netlink_diag.h
index 4e31db4eea41..d15d5c8f9d9c 100644
--- a/include/uapi/linux/netlink_diag.h
+++ b/include/uapi/linux/netlink_diag.h
@@ -33,6 +33,7 @@ struct netlink_diag_ring {
 };
 
 enum {
+	NETLINK_DIAG_NONE,
 	NETLINK_DIAG_MEMINFO,
 	NETLINK_DIAG_GROUPS,
 	NETLINK_DIAG_RX_RING,
diff --git a/include/uapi/linux/packet_diag.h b/include/uapi/linux/packet_diag.h
index b2cc0cd9c4d9..e49fa94b3463 100644
--- a/include/uapi/linux/packet_diag.h
+++ b/include/uapi/linux/packet_diag.h
@@ -29,6 +29,7 @@ struct packet_diag_msg {
 };
 
 enum {
+	PACKET_DIAG_UNSPEC,
 	PACKET_DIAG_INFO,
 	PACKET_DIAG_MCLIST,
 	PACKET_DIAG_RX_RING,
diff --git a/include/uapi/linux/unix_diag.h b/include/uapi/linux/unix_diag.h
index b9e2a6a7446f..b24e9626c704 100644
--- a/include/uapi/linux/unix_diag.h
+++ b/include/uapi/linux/unix_diag.h
@@ -31,6 +31,7 @@ struct unix_diag_msg {
 };
 
 enum {
+	UNIX_DIAG_NONE,
 	UNIX_DIAG_NAME,
 	UNIX_DIAG_VFS,
 	UNIX_DIAG_PEER,
-- 
1.8.4.1

Re: [PATCH net] diag: fix netlink API attributes

[Thomas Graf]

On 11/28/13 at 02:57pm, Nicolas Dichtel wrote:
> The first netlink attribute (value 0) must always be defined as none/unspec.
> This is correctly done in inet_diag.h, but other diag interfaces are broken.
> 
> Libraries like libnl skip this kind of attributes, thus it's never reported to
> the application.
> 
> CC: Thomas Graf <tgraf@suug.ch>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

First of all, thanks for the notification Nicolas. I'll fix libnl to
pass through these attributes.

I think the fix you propose is no an option as it breaks backwards
compatiblity with existing user space. We cannot change the value
of existing attributes.

[RFC PATCH v2] diag: warn about missing first netlink attribute

[Nicolas Dichtel]

The first netlink attribute (value 0) must always be defined as none/unspec.
This is correctly done in inet_diag.h, but other diag interfaces are wrong.

Because we cannot change an existing API, I add a comment to point the mistake
and avoid to propagate it in a new diag API in the future.

CC: Thomas Graf <tgraf@suug.ch>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---

v2: just warn about the missing attribute

If patch is approved, I will submit it when the net-next tree opens.

 include/uapi/linux/netlink_diag.h | 1 +
 include/uapi/linux/packet_diag.h  | 1 +
 include/uapi/linux/unix_diag.h    | 1 +
 3 files changed, 3 insertions(+)

diff --git a/include/uapi/linux/netlink_diag.h b/include/uapi/linux/netlink_diag.h
index 4e31db4eea41..f2159d30d1f5 100644
--- a/include/uapi/linux/netlink_diag.h
+++ b/include/uapi/linux/netlink_diag.h
@@ -33,6 +33,7 @@ struct netlink_diag_ring {
 };
 
 enum {
+	/* NETLINK_DIAG_NONE, standard nl API requires this attribute!  */
 	NETLINK_DIAG_MEMINFO,
 	NETLINK_DIAG_GROUPS,
 	NETLINK_DIAG_RX_RING,
diff --git a/include/uapi/linux/packet_diag.h b/include/uapi/linux/packet_diag.h
index b2cc0cd9c4d9..d08c63f3dd6f 100644
--- a/include/uapi/linux/packet_diag.h
+++ b/include/uapi/linux/packet_diag.h
@@ -29,6 +29,7 @@ struct packet_diag_msg {
 };
 
 enum {
+	/* PACKET_DIAG_NONE, standard nl API requires this attribute!  */
 	PACKET_DIAG_INFO,
 	PACKET_DIAG_MCLIST,
 	PACKET_DIAG_RX_RING,
diff --git a/include/uapi/linux/unix_diag.h b/include/uapi/linux/unix_diag.h
index b9e2a6a7446f..1eb0b8dd1830 100644
--- a/include/uapi/linux/unix_diag.h
+++ b/include/uapi/linux/unix_diag.h
@@ -31,6 +31,7 @@ struct unix_diag_msg {
 };
 
 enum {
+	/* UNIX_DIAG_NONE, standard nl API requires this attribute!  */
 	UNIX_DIAG_NAME,
 	UNIX_DIAG_VFS,
 	UNIX_DIAG_PEER,
-- 
1.8.4.1

Re: [PATCH net] diag: fix netlink API attributes

[Nicolas Dichtel]

Le 28/11/2013 17:38, Thomas Graf a écrit :
> On 11/28/13 at 02:57pm, Nicolas Dichtel wrote:
>> The first netlink attribute (value 0) must always be defined as none/unspec.
>> This is correctly done in inet_diag.h, but other diag interfaces are broken.
>>
>> Libraries like libnl skip this kind of attributes, thus it's never reported to
>> the application.
>>
>> CC: Thomas Graf <tgraf@suug.ch>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>
> First of all, thanks for the notification Nicolas. I'll fix libnl to
> pass through these attributes.
Fine :)
Thank you!

>
> I think the fix you propose is no an option as it breaks backwards
> compatiblity with existing user space. We cannot change the value
> of existing attributes.
Yes, I was just wondering how to fix it.
It also breaks the patch you've proposed in this thread:
http://thread.gmane.org/gmane.linux.network/251478/focus=252764

Hence, aligning attributes on 64-bits is still unresolved :(

Re: [RFC PATCH v2] diag: warn about missing first netlink attribute

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Thu, 28 Nov 2013 18:31:05 +0100

> The first netlink attribute (value 0) must always be defined as none/unspec.
> This is correctly done in inet_diag.h, but other diag interfaces are wrong.
> 
> Because we cannot change an existing API, I add a comment to point the mistake
> and avoid to propagate it in a new diag API in the future.
> 
> CC: Thomas Graf <tgraf@suug.ch>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
> 
> v2: just warn about the missing attribute
> 
> If patch is approved, I will submit it when the net-next tree opens.

Indeed we can't change it, and documenting it like this is also a good
idea.

I hope Thomas agrees.

Re: [RFC PATCH v2] diag: warn about missing first netlink attribute

[Thomas Graf]

On 11/28/13 at 06:31pm, Nicolas Dichtel wrote:
> The first netlink attribute (value 0) must always be defined as none/unspec.
> This is correctly done in inet_diag.h, but other diag interfaces are wrong.
> 
> Because we cannot change an existing API, I add a comment to point the mistake
> and avoid to propagate it in a new diag API in the future.
> 
> CC: Thomas Graf <tgraf@suug.ch>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
> 
> v2: just warn about the missing attribute
> 
> If patch is approved, I will submit it when the net-next tree opens.

Acked-by: Thomas Graf <tgraf@suug.ch>

Re: [PATCH net] diag: fix netlink API attributes

[Thomas Graf]

On 11/28/13 at 06:37pm, Nicolas Dichtel wrote:
> Le 28/11/2013 17:38, Thomas Graf a écrit :
> >On 11/28/13 at 02:57pm, Nicolas Dichtel wrote:
> >>The first netlink attribute (value 0) must always be defined as none/unspec.
> >>This is correctly done in inet_diag.h, but other diag interfaces are broken.
> >>
> >>Libraries like libnl skip this kind of attributes, thus it's never reported to
> >>the application.
> >>
> >>CC: Thomas Graf <tgraf@suug.ch>
> >>Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> >
> >First of all, thanks for the notification Nicolas. I'll fix libnl to
> >pass through these attributes.
> Fine :)
> Thank you!

Fixed in:

commit 6a8d90f5fec48b6e376ff29ccf3e0c620a41e758
Author: Thomas Graf <tgraf@suug.ch>
Date:   Thu Nov 28 23:14:38 2013 +0100

    attr: Allow attribute type 0

    {netlink,packet,unix}_diag use attribute type 0 for valid
    attributes. The value was reserved and usage was prohibited
    by the protocol but we can't undo the breakge.
    
    Make libnl accept attribute type 0 to allow parsing these
    attributes.
    
    Reported-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
    Signed-off-by: Thomas Graf <tgraf@suug.ch>

> >I think the fix you propose is no an option as it breaks backwards
> >compatiblity with existing user space. We cannot change the value
> >of existing attributes.
> Yes, I was just wondering how to fix it.
> It also breaks the patch you've proposed in this thread:
> http://thread.gmane.org/gmane.linux.network/251478/focus=252764
> 
> Hence, aligning attributes on 64-bits is still unresolved :(

Unfortunately I ran into other places where an attribute type 0
was used. Therefore the idea of using NLA_UNSPEC (0) to insert
padding attributes doesn't work. We would need to find another
way to insert padding. I'll be glad to share the WIP patch if
somebody else would like to continue the work.

Re: [RFC PATCH v2] diag: warn about missing first netlink attribute

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Thu, 28 Nov 2013 22:09:09 +0000

> On 11/28/13 at 06:31pm, Nicolas Dichtel wrote:
>> The first netlink attribute (value 0) must always be defined as none/unspec.
>> This is correctly done in inet_diag.h, but other diag interfaces are wrong.
>> 
>> Because we cannot change an existing API, I add a comment to point the mistake
>> and avoid to propagate it in a new diag API in the future.
>> 
>> CC: Thomas Graf <tgraf@suug.ch>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> ---
>> 
>> v2: just warn about the missing attribute
>> 
>> If patch is approved, I will submit it when the net-next tree opens.
> 
> Acked-by: Thomas Graf <tgraf@suug.ch>

Applied, thanks.

Re: [PATCH net] diag: fix netlink API attributes

[Nicolas Dichtel]

Le 28/11/2013 23:19, Thomas Graf a écrit :
> On 11/28/13 at 06:37pm, Nicolas Dichtel wrote:
>> Le 28/11/2013 17:38, Thomas Graf a écrit :
>>> On 11/28/13 at 02:57pm, Nicolas Dichtel wrote:
>>>> The first netlink attribute (value 0) must always be defined as none/unspec.
>>>> This is correctly done in inet_diag.h, but other diag interfaces are broken.
>>>>
>>>> Libraries like libnl skip this kind of attributes, thus it's never reported to
>>>> the application.
>>>>
>>>> CC: Thomas Graf <tgraf@suug.ch>
>>>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>>>
>>> First of all, thanks for the notification Nicolas. I'll fix libnl to
>>> pass through these attributes.
>> Fine :)
>> Thank you!
>
> Fixed in:
>
> commit 6a8d90f5fec48b6e376ff29ccf3e0c620a41e758
> Author: Thomas Graf <tgraf@suug.ch>
> Date:   Thu Nov 28 23:14:38 2013 +0100
>
>      attr: Allow attribute type 0
>
>      {netlink,packet,unix}_diag use attribute type 0 for valid
>      attributes. The value was reserved and usage was prohibited
>      by the protocol but we can't undo the breakge.
>
>      Make libnl accept attribute type 0 to allow parsing these
>      attributes.
>
>      Reported-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>      Signed-off-by: Thomas Graf <tgraf@suug.ch>
I think the following patch is also needed (not based on HEAD):

 From 8aa4397d43fbd34deea9ed11677c04b460895f15 Mon Sep 17 00:00:00 2001
From: Samuel Gauthier <samuel.gauthier@6wind.com>
Date: Fri, 29 Nov 2013 09:15:34 +0100
Subject: [PATCH] attr: Allow attribute type 0 parsing

The commit 6a8d90f5fec4 "attr: Allow attribute type 0" intended to
allow the parsing of {netlink,packet,unix}_diag, even if they are
using type 0 for valid attributes.

It lacked this part in nla_parse.

Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>
---
  lib/attr.c | 4 ----
  1 file changed, 4 deletions(-)

diff --git a/lib/attr.c b/lib/attr.c
index 535f10c..1e2d57f 100644
--- a/lib/attr.c
+++ b/lib/attr.c
@@ -250,10 +250,6 @@ int nla_parse(struct nlattr *tb[], int maxtype, struct 
nlattr *head, int len,
  	nla_for_each_attr(nla, head, len, rem) {
  		int type = nla_type(nla);

-		/* Padding attributes */
-		if (type == 0)
-			continue;
-
  		if (type > maxtype)
  			continue;

-- 
1.8.0

Re: [PATCH net] diag: fix netlink API attributes

[Thomas Graf]

On 11/29/13 at 09:28am, Nicolas Dichtel wrote:
> I think the following patch is also needed (not based on HEAD):
> 
> From 8aa4397d43fbd34deea9ed11677c04b460895f15 Mon Sep 17 00:00:00 2001
> From: Samuel Gauthier <samuel.gauthier@6wind.com>
> Date: Fri, 29 Nov 2013 09:15:34 +0100
> Subject: [PATCH] attr: Allow attribute type 0 parsing
> 
> The commit 6a8d90f5fec4 "attr: Allow attribute type 0" intended to
> allow the parsing of {netlink,packet,unix}_diag, even if they are
> using type 0 for valid attributes.
> 
> It lacked this part in nla_parse.
> 
> Signed-off-by: Samuel Gauthier <samuel.gauthier@6wind.com>

Good catch, applied, thanks!