* [PATCH net] diag: fix netlink API attributes
@ 2013-11-28 13:57 Nicolas Dichtel
2013-11-28 16:38 ` Thomas Graf
0 siblings, 1 reply; 10+ messages in thread
From: Nicolas Dichtel @ 2013-11-28 13:57 UTC (permalink / raw)
To: davem; +Cc: netdev, Nicolas Dichtel, Thomas Graf
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH net] diag: fix netlink API attributes
2013-11-28 13:57 [PATCH net] diag: fix netlink API attributes Nicolas Dichtel
@ 2013-11-28 16:38 ` Thomas Graf
2013-11-28 17:31 ` [RFC PATCH v2] diag: warn about missing first netlink attribute Nicolas Dichtel
2013-11-28 17:37 ` [PATCH net] diag: fix netlink API attributes Nicolas Dichtel
0 siblings, 2 replies; 10+ messages in thread
From: Thomas Graf @ 2013-11-28 16:38 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: davem, netdev
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [RFC PATCH v2] diag: warn about missing first netlink attribute
2013-11-28 16:38 ` Thomas Graf
@ 2013-11-28 17:31 ` Nicolas Dichtel
2013-11-28 21:36 ` David Miller
2013-11-28 22:09 ` Thomas Graf
2013-11-28 17:37 ` [PATCH net] diag: fix netlink API attributes Nicolas Dichtel
1 sibling, 2 replies; 10+ messages in thread
From: Nicolas Dichtel @ 2013-11-28 17:31 UTC (permalink / raw)
To: tgraf; +Cc: davem, netdev, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [RFC PATCH v2] diag: warn about missing first netlink attribute
2013-11-28 17:31 ` [RFC PATCH v2] diag: warn about missing first netlink attribute Nicolas Dichtel
@ 2013-11-28 21:36 ` David Miller
2013-11-28 22:09 ` Thomas Graf
1 sibling, 0 replies; 10+ messages in thread
From: David Miller @ 2013-11-28 21:36 UTC (permalink / raw)
To: nicolas.dichtel; +Cc: tgraf, netdev
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH v2] diag: warn about missing first netlink attribute
2013-11-28 17:31 ` [RFC PATCH v2] diag: warn about missing first netlink attribute Nicolas Dichtel
2013-11-28 21:36 ` David Miller
@ 2013-11-28 22:09 ` Thomas Graf
2013-11-28 23:16 ` David Miller
1 sibling, 1 reply; 10+ messages in thread
From: Thomas Graf @ 2013-11-28 22:09 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: davem, netdev
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH v2] diag: warn about missing first netlink attribute
2013-11-28 22:09 ` Thomas Graf
@ 2013-11-28 23:16 ` David Miller
0 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2013-11-28 23:16 UTC (permalink / raw)
To: tgraf; +Cc: nicolas.dichtel, netdev
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] diag: fix netlink API attributes
2013-11-28 16:38 ` Thomas Graf
2013-11-28 17:31 ` [RFC PATCH v2] diag: warn about missing first netlink attribute Nicolas Dichtel
@ 2013-11-28 17:37 ` Nicolas Dichtel
2013-11-28 22:19 ` Thomas Graf
1 sibling, 1 reply; 10+ messages in thread
From: Nicolas Dichtel @ 2013-11-28 17:37 UTC (permalink / raw)
To: Thomas Graf; +Cc: davem, netdev
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 :(
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net] diag: fix netlink API attributes
2013-11-28 17:37 ` [PATCH net] diag: fix netlink API attributes Nicolas Dichtel
@ 2013-11-28 22:19 ` Thomas Graf
2013-11-29 8:28 ` Nicolas Dichtel
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Graf @ 2013-11-28 22:19 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: davem, netdev
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.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH net] diag: fix netlink API attributes
2013-11-28 22:19 ` Thomas Graf
@ 2013-11-29 8:28 ` Nicolas Dichtel
2013-11-29 8:42 ` Thomas Graf
0 siblings, 1 reply; 10+ messages in thread
From: Nicolas Dichtel @ 2013-11-29 8:28 UTC (permalink / raw)
To: Thomas Graf; +Cc: davem, netdev, Samuel.gauthier@6wind•com
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH net] diag: fix netlink API attributes
2013-11-29 8:28 ` Nicolas Dichtel
@ 2013-11-29 8:42 ` Thomas Graf
0 siblings, 0 replies; 10+ messages in thread
From: Thomas Graf @ 2013-11-29 8:42 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: davem, netdev, Samuel.gauthier@6wind•com
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!
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-11-29 8:42 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-28 13:57 [PATCH net] diag: fix netlink API attributes Nicolas Dichtel
2013-11-28 16:38 ` Thomas Graf
2013-11-28 17:31 ` [RFC PATCH v2] diag: warn about missing first netlink attribute Nicolas Dichtel
2013-11-28 21:36 ` David Miller
2013-11-28 22:09 ` Thomas Graf
2013-11-28 23:16 ` David Miller
2013-11-28 17:37 ` [PATCH net] diag: fix netlink API attributes Nicolas Dichtel
2013-11-28 22:19 ` Thomas Graf
2013-11-29 8:28 ` Nicolas Dichtel
2013-11-29 8:42 ` Thomas Graf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox