Skip to contentSkip to conversation

inbound connection problems when "netlink: test for all flags of the NLM_F_DUMP composite" commit applied

DISCUSSION Arthur Marsh 7 participants 31 messages Jan 16, 2011 – Jan 19, 2011 1 message not in the archive

Subsystems (4)
Networking Network drivers netfilter Networking IPsec
lore
Share & tools
lore

Arthur Marsh

author Acked-by Pablo Neira Ayuso
#0 lore raw

This bug was originally posted at https://bugzilla.kernel.org/show_bug.cgi?id=26632

With kernels up to and including 2.6.37-git7, inbound telnetd-ssl connections worked fine.
With kernel 2.6.37-git9 and later inbound telnetd-ssl connections failed, and on machine shut-down, there were warning messages about daemons not return status.

A git bisect on Linus' kernel tree revealed:

0ab03c2b1478f2438d2c80204f7fef65b1bca9cf is the first bad commit commit 0ab03c2b1478f2438d2c80204f7fef65b1bca9cf

Author: Jan Engelhardt <[email protected]> 
Date:   Fri Jan 7 03:15:05 2011 +0000

netlink: test for all flags of the NLM_F_DUMP composite

Due to NLM_F_DUMP is composed of two bits, NLM_F_ROOT | NLM_F_MATCH, when doing "if (x & NLM_F_DUMP)", it tests for _either_ of the bits being set. Because NLM_F_MATCH's value overlaps with NLM_F_EXCL, non-dump requests with NLM_F_EXCL set are mistaken as dump requests.

Substitute the condition to test for _all_ bits being set.

Signed-off-by Jan Engelhardt <[email protected]>
Acked-by Pablo Neira Ayuso <[email protected]>
Signed-off-by David S. Miller <[email protected]>

:040000 040000 1a0717ab0c87787309c3c3af88d666b44f327f64
cba6279de85b7ebeaf21f19f1d93b59468fdd01d M net

I tried git cherry-pick 0ab03c2b1478f2438d2c80204f7fef65b1bca9cf and verified that the resulting kernel had these problems, then git revert 0ab03c2b1478f2438d2c80204f7fef65b1bca9cf and verified that the resulting kernel did *not* have problems.

Arthur.

#1 lore raw

Le dimanche 16 janvier 2011 à 19:24 +1030, Arthur Marsh a écrit :

CC people involved with the commit

Quoted 36 lines

This bug was originally posted at https://bugzilla.kernel.org/show_bug.cgi?id=26632

With kernels up to and including 2.6.37-git7, inbound telnetd-ssl connections worked fine.
With kernel 2.6.37-git9 and later inbound telnetd-ssl connections failed, and on machine shut-down, there were warning messages about daemons not return status.

A git bisect on Linus' kernel tree revealed:

0ab03c2b1478f2438d2c80204f7fef65b1bca9cf is the first bad commit commit 0ab03c2b1478f2438d2c80204f7fef65b1bca9cf

Author: Jan Engelhardt <[email protected]> 
Date:   Fri Jan 7 03:15:05 2011 +0000

netlink: test for all flags of the NLM_F_DUMP composite

Due to NLM_F_DUMP is composed of two bits, NLM_F_ROOT | NLM_F_MATCH, when doing "if (x & NLM_F_DUMP)", it tests for _either_ of the bits being set. Because NLM_F_MATCH's value overlaps with NLM_F_EXCL, non-dump requests with NLM_F_EXCL set are mistaken as dump requests.

Substitute the condition to test for _all_ bits being set.

Signed-off-by Jan Engelhardt <[email protected]>
Acked-by Pablo Neira Ayuso <[email protected]>
Signed-off-by David S. Miller <[email protected]>

:040000 040000 1a0717ab0c87787309c3c3af88d666b44f327f64
cba6279de85b7ebeaf21f19f1d93b59468fdd01d M net

I tried git cherry-pick 0ab03c2b1478f2438d2c80204f7fef65b1bca9cf and verified that the resulting kernel had these problems, then git revert 0ab03c2b1478f2438d2c80204f7fef65b1bca9cf and verified that the resulting kernel did *not* have problems.

Arthur.

#2 lore raw
Quoted 5 lines · Arthur Marsh
Quoted 5 lines

With kernels up to and including 2.6.37-git7, inbound telnetd-ssl connections worked fine. With kernel 2.6.37-git9 and later inbound telnetd-ssl connections failed, and on machine shut-down, there were warning messages about daemons not return status.

Which daemons are these? For reference, what distro do you happen to use?

Quoted 2 lines
Quoted 2 lines

commit 0ab03c2b1478f2438d2c80204f7fef65b1bca9cf
netlink: test for all flags of the NLM_F_DUMP composite

Each of the hunks in this commit is independent of another. Would you mind bisecting these too?

author
#3 lore raw

Jan Engelhardt wrote, on 16/01/11 21:20:

Quoted 10 lines · Arthur Marsh
Quoted 5 lines
Quoted 5 lines

With kernels up to and including 2.6.37-git7, inbound telnetd-ssl connections worked fine. With kernel 2.6.37-git9 and later inbound telnetd-ssl connections failed, and on machine shut-down, there were warning messages about daemons not return status.

Which daemons are these? For reference, what distro do you happen to use?

avahi-daemon (which gave multiple warning messages, hence I thought it may have been multiple packages)

I'm running Debian unstable with kernel.org kernels.

Quoted 6 lines
Quoted 3 lines
Quoted 3 lines

commit 0ab03c2b1478f2438d2c80204f7fef65b1bca9cf
netlink: test for all flags of the NLM_F_DUMP composite

Each of the hunks in this commit is independent of another. Would you mind bisecting these too?

Recompiling with the only the first patch (attached) resulted in a repeat of the problem.

Arthur.

net/core/rtnetlink.c +1 -1
index 750db57..a5f7535 100644
@@ -1820,7 +1820,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 	if (kind != 2 && security_netlink_recv(skb, CAP_NET_ADMIN))
 		return -EPERM;
 
-	if (kind == 2 && nlh->nlmsg_flags&NLM_F_DUMP) {
+	if (kind == 2 && (nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) {
 		struct sock *rtnl;
 		rtnl_dumpit_func dumpit;

A message that is not in the archive (sent off-list or not yet received): [email protected]

maintainer
#5 lore raw
Quoted 21 lines · Arthur Marsh

Jan Engelhardt wrote, on 16/01/11 21:20:

Quoted 10 lines · Arthur Marsh
Quoted 5 lines
Quoted 5 lines

With kernels up to and including 2.6.37-git7, inbound telnetd-ssl connections worked fine. With kernel 2.6.37-git9 and later inbound telnetd-ssl connections failed, and on machine shut-down, there were warning messages about daemons not return status.

Which daemons are these? For reference, what distro do you happen to use?

avahi-daemon (which gave multiple warning messages, hence I thought it may have been multiple packages)

I'm running Debian unstable with kernel.org kernels.

Quoted 4 lines
Quoted 3 lines
Quoted 3 lines

commit 0ab03c2b1478f2438d2c80204f7fef65b1bca9cf
netlink: test for all flags of the NLM_F_DUMP composite

Quoted 8 lines
Quoted 2 lines

Each of the hunks in this commit is independent of another. Would you mind bisecting these too?

Recompiling with the only the first patch (attached) resulted in a repeat of the problem.

I've removed one person from the cc: list as they did not want to receive email about this even though they signed off the commit.

Please, pass this patch to the avahi-daemon developers. They use an invalid netlink flag combination for dump operations.

This patch has spotted a problem that they have to fix indeed.

avahi: fix wrong use of netlink flags for dump operations

The avahi-daemon uses a wrong flag combination to operate with rtnetlink. This patch fixes the problems.

No need to set NLM_F_ACK since the dump operation already includes the trailing NLMSG_DONE message that informs about the end of the dump operation.

Please, consider porting the avahi-daemon to libmnl:

http://www.netfilter.org/projects/libmnl/index.html

Signed-off-by Pablo Neira Ayuso <[email protected]>
avahi-autoipd/iface-linux.c +1 -1
Index: avahi-0.6.27/avahi-autoipd/iface-linux.c
===================================================================
@@ -262,7 +262,7 @@ int iface_get_initial_state(State *state
     n->nlmsg_len = NLMSG_LENGTH(sizeof(*ifi));
     n->nlmsg_type = RTM_GETLINK;
     n->nlmsg_seq = seq;
-    n->nlmsg_flags = NLM_F_MATCH|NLM_F_REQUEST|NLM_F_ACK;
+    n->nlmsg_flags = NLM_F_REQUEST|NLM_F_DUMP;
     n->nlmsg_pid = 0;
 
     ifi = NLMSG_DATA(n);
avahi-core/iface-linux.c +1 -1
Index: avahi-0.6.27/avahi-core/iface-linux.c
===================================================================
@@ -53,7 +53,7 @@ static int netlink_list_items(AvahiNetli
     n = (struct nlmsghdr*) req;
     n->nlmsg_len = NLMSG_LENGTH(sizeof(struct rtgenmsg));
     n->nlmsg_type = type;
-    n->nlmsg_flags = NLM_F_ROOT|NLM_F_REQUEST;
+    n->nlmsg_flags = NLM_F_REQUEST|NLM_F_DUMP;
     n->nlmsg_pid = 0;
 
     gen = NLMSG_DATA(n);
author
#6 lore raw

Pablo Neira Ayuso wrote, on 17/01/11 07:47:

Quoted 28 lines · Arthur Marsh
Quoted 27 lines

Jan Engelhardt wrote, on 16/01/11 21:20:

Quoted 10 lines · Arthur Marsh
Quoted 5 lines
Quoted 5 lines

With kernels up to and including 2.6.37-git7, inbound telnetd-ssl connections worked fine. With kernel 2.6.37-git9 and later inbound telnetd-ssl connections failed, and on machine shut-down, there were warning messages about daemons not return status.

Which daemons are these? For reference, what distro do you happen to use?

avahi-daemon (which gave multiple warning messages, hence I thought it may have been multiple packages)

I'm running Debian unstable with kernel.org kernels.

Quoted 6 lines
Quoted 3 lines
Quoted 3 lines

commit 0ab03c2b1478f2438d2c80204f7fef65b1bca9cf
netlink: test for all flags of the NLM_F_DUMP composite

Each of the hunks in this commit is independent of another. Would you mind bisecting these too?

Recompiling with the only the first patch (attached) resulted in a repeat of the problem.

Quoted 8 lines
Quoted 2 lines

I've removed one person from the cc: list as they did not want to receive email about this even though they signed off the commit.

Please, pass this patch to the avahi-daemon developers. They use an invalid netlink flag combination for dump operations.

This patch has spotted a problem that they have to fix indeed.

I've forwarded this on as a Debian bug:

http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=610281

Regards,

Arthur.

#7 lore raw
In reply to Pablo Neira Ayuso
Quoted 25 lines · Pablo Neira Ayuso
Quoted 24 lines · Arthur Marsh

Jan Engelhardt wrote, on 16/01/11 21:20:

Quoted 10 lines

Le dimanche 16 janvier 2011 Ă 19:24 +1030, Arthur Marsh a ĂŠcrit :

Quoted 5 lines
Quoted 5 lines

With kernels up to and including 2.6.37-git7, inbound telnetd-ssl connections worked fine. With kernel 2.6.37-git9 and later inbound telnetd-ssl connections failed, and on machine shut-down, there were warning messages about daemons not return status.

Which daemons are these? For reference, what distro do you happen to use?

avahi-daemon (which gave multiple warning messages, hence I thought it may have been multiple packages)

I'm running Debian unstable with kernel.org kernels.

Quoted 6 lines
Quoted 3 lines
Quoted 3 lines

commit 0ab03c2b1478f2438d2c80204f7fef65b1bca9cf
netlink: test for all flags of the NLM_F_DUMP composite

Each of the hunks in this commit is independent of another. Would you mind bisecting these too?

Quoted 8 lines
Quoted 5 lines

Recompiling with the only the first patch (attached) resulted in a repeat of the problem.

I've removed one person from the cc: list as they did not want to receive email about this even though they signed off the commit.

Please, pass this patch to the avahi-daemon developers. They use an invalid netlink flag combination for dump operations.

Nothing in RFC suggests this is an invalid netlink flag combination, and author's implementation had suported it:
ftp://ftp.rfc-editor.org/in-notes/rfc3549.txt

NLM_F_DUMP is called a convenience macro only, so might be interpreted as a special kind of dump. BTW, isn't NLM_F_ATOMIC flag with NLM_F_DUMP treated as invalid now either?

Even if I'm wrong, this change added to stable will break many configs. My proposal is to revert commit 0ab03c2b147 until proper fix is found.

Cc Jamal Hadi Salim <[email protected]>

Jarek P.

maintainer
#8 lore raw

From: Jarek Poplawski <[email protected]>
Date: Tue, 18 Jan 2011 09:38:11 +0000

Quoted 2 lines

Even if I'm wrong, this change added to stable will break many configs. My proposal is to revert commit 0ab03c2b147 until proper fix is found.

The flag combination is, at best ambiguous, it has no proper definition without the check we added.

#9 lore raw
Quoted 8 lines · David Miller

From: Jarek Poplawski <[email protected]>
Date: Tue, 18 Jan 2011 09:38:11 +0000

Quoted 2 lines

Even if I'm wrong, this change added to stable will break many configs. My proposal is to revert commit 0ab03c2b147 until proper fix is found.

The flag combination is, at best ambiguous, it has no proper definition without the check we added.

Do you all expect all users manage to upgrade avahi app before changing their stable kernel? I mean "own distro" users especially.

Jarek P.

#10 lore raw
Quoted 2 lines · Jarek Poplawski

Do you all expect all users manage to upgrade avahi app before changing their stable kernel? I mean "own distro" users especially.

Unfortunately if that app is widely deployed, it is not pragmatic to break it in the name of pedanticity. i.e.
Be conservative in what you send (clearly Avahi is farting all over the place) but more importantly be a nice liberal in what you accept. Maybe tell them if you have the cycles about their bad behavior. The important part is the GET (kind = 2). The DUMP as Jarek says is merely a utility to extrapolate that we need you to
get everything.. So it is not such a big deal if someone passes extraneous senseless flags. Wrong? yes.

Jarek - For the record although i coauthored the doc, I was merely a messenger in putting together some artist painting.

cheers,
jamal

#11 lore raw
Quoted 7 lines · jamal
Quoted 2 lines · Jarek Poplawski

Do you all expect all users manage to upgrade avahi app before changing their stable kernel? I mean "own distro" users especially.

Unfortunately if that app is widely deployed, it is not pragmatic to break it in the name of pedanticity.

And to complete that thought - if avahi continues to work and merely whines, i dont see why this patch should be reverted..

cheers,
jamal

#12 lore raw
Quoted 11 lines · jamal
Quoted 7 lines · jamal
Quoted 2 lines · Jarek Poplawski

Do you all expect all users manage to upgrade avahi app before changing their stable kernel? I mean "own distro" users especially.

Unfortunately if that app is widely deployed, it is not pragmatic to break it in the name of pedanticity.

And to complete that thought - if avahi continues to work and merely whines, i dont see why this patch should be reverted..

Alas it looks like more than whines ;-)
Cheers,
Jarek P.

[PATCH] netlink: Revert test for all flags of the NLM_F_DUMP composite

Arthur Marsh reported:

Quoted 3 lines

With kernel 2.6.37-git9 and later inbound telnetd-ssl connections failed, and on machine shut-down, there were warning messages about daemons not return status.

and bisected the bug.

After looking at net/core/rtnetlink.c:

static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)

{
	...
        kind = type&3;

        if (kind != 2 && security_netlink_recv(skb, CAP_NET_ADMIN))
                return -EPERM;

        if (kind == 2 && nlh->nlmsg_flags&NLM_F_DUMP) {

I'm sure the fix in commit 0ab03c2b1478f24 is simply wrong. NLM_F_DUMP flags can't be mistaken with NLM_F_EXCL if there is a check for GET request like above (ie. kind == 2). So there is no reason to limit 3 various dump cases from the RFC (not counting the atomic flag) to one and only NLM_F_DUMP.

That's why I propose this reverting patch here and a separate fix to genetlink (soon).

Reverts commit 0ab03c2b1478f2438d2c80204f7fef65b1bca9cf

Reported-by Alessandro Suardi <[email protected]>
Reported-by Arthur Marsh <[email protected]>
Bisected-by Arthur Marsh <[email protected]>
Signed-off-by Jarek Poplawski <[email protected]>
Cc Jan Engelhardt <[email protected]>
Cc Pablo Neira Ayuso <[email protected]>
Cc Jamal Hadi Salim <[email protected]>

---

 net/core/rtnetlink.c                 |    2 +-
 net/ipv4/inet_diag.c                 |    2 +-
 net/netfilter/nf_conntrack_netlink.c |    4 ++--
 net/netlink/genetlink.c              |    2 +-
 net/xfrm/xfrm_user.c                 |    2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)
net/core/rtnetlink.c +1 -1
index a5f7535..750db57 100644
@@ -1820,7 +1820,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 	if (kind != 2 && security_netlink_recv(skb, CAP_NET_ADMIN))
 		return -EPERM;
 
-	if (kind == 2 && (nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) {
+	if (kind == 2 && nlh->nlmsg_flags&NLM_F_DUMP) {
 		struct sock *rtnl;
 		rtnl_dumpit_func dumpit;
net/ipv4/inet_diag.c +1 -1
index 2746c1f..2ada171 100644
@@ -858,7 +858,7 @@ static int inet_diag_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 	    nlmsg_len(nlh) < hdrlen)
 		return -EINVAL;
 
-	if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) {
+	if (nlh->nlmsg_flags & NLM_F_DUMP) {
 		if (nlmsg_attrlen(nlh, hdrlen)) {
 			struct nlattr *attr;
net/netfilter/nf_conntrack_netlink.c +2 -2
index 2b7eef3..93297aa 100644
@@ -924,7 +924,7 @@ ctnetlink_get_conntrack(struct sock *ctnl, struct sk_buff *skb,
 	u16 zone;
 	int err;
 
-	if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP)
+	if (nlh->nlmsg_flags & NLM_F_DUMP)
 		return netlink_dump_start(ctnl, skb, nlh, ctnetlink_dump_table,
 					  ctnetlink_done);
 

@@ -1787,7 +1787,7 @@ ctnetlink_get_expect(struct sock *ctnl, struct sk_buff *skb,
 	u16 zone;
 	int err;
 
-	if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) {
+	if (nlh->nlmsg_flags & NLM_F_DUMP) {
 		return netlink_dump_start(ctnl, skb, nlh,
 					  ctnetlink_exp_dump_table,
 					  ctnetlink_exp_done);
net/netlink/genetlink.c +1 -1
index f83cb37..1781d99 100644
@@ -519,7 +519,7 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 	    security_netlink_recv(skb, CAP_NET_ADMIN))
 		return -EPERM;
 
-	if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) {
+	if (nlh->nlmsg_flags & NLM_F_DUMP) {
 		if (ops->dumpit == NULL)
 			return -EOPNOTSUPP;
net/xfrm/xfrm_user.c +1 -1
index d5e1e0b..6129196 100644
@@ -2189,7 +2189,7 @@ static int xfrm_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 
 	if ((type == (XFRM_MSG_GETSA - XFRM_MSG_BASE) ||
 	     type == (XFRM_MSG_GETPOLICY - XFRM_MSG_BASE)) &&
-	    (nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) {
+	    (nlh->nlmsg_flags & NLM_F_DUMP)) {
 		if (link->dump == NULL)
 			return -EINVAL;
#13 lore raw
In reply to jamal
Quoted 5 lines · jamal
Quoted 2 lines · Jarek Poplawski

Do you all expect all users manage to upgrade avahi app before changing their stable kernel? I mean "own distro" users especially.

Quoted 9 lines

Unfortunately if that app is widely deployed, it is not pragmatic to break it in the name of pedanticity. i.e.
Be conservative in what you send (clearly Avahi is farting all over the place) but more importantly be a nice liberal in what you accept. Maybe tell them if you have the cycles about their bad behavior. The important part is the GET (kind = 2). The DUMP as Jarek says is merely a utility to extrapolate that we need you to
get everything.. So it is not such a big deal if someone passes extraneous senseless flags. Wrong? yes.

Thanks for the confirmation of my suspicions wrt. the RFC & rtnetlink. But then Avahi seems right and we should get back to the written law.

Quoted 2 lines

Jarek - For the record although i coauthored the doc, I was merely a messenger in putting together some artist painting.

Well, ain't pictures better than words ;-)

Cheers,
Jarek P.

maintainer
#14 lore raw
In reply to jamal

From: jamal <[email protected]>
Date: Tue, 18 Jan 2011 09:05:03 -0500

Quoted 5 lines · Jarek Poplawski
Quoted 3 lines

Do you all expect all users manage to upgrade avahi app before changing their stable kernel? I mean "own distro" users especially.

Quoted 9 lines

Unfortunately if that app is widely deployed, it is not pragmatic to break it in the name of pedanticity. i.e.
Be conservative in what you send (clearly Avahi is farting all over the place) but more importantly be a nice liberal in what you accept. Maybe tell them if you have the cycles about their bad behavior. The important part is the GET (kind = 2). The DUMP as Jarek says is merely a utility to extrapolate that we need you to
get everything.. So it is not such a big deal if someone passes extraneous senseless flags. Wrong? yes.

Fair enough, I'll revert the netlink patch.

maintainer
#15 lore raw
In reply to Jarek Poplawski
Quoted 4 lines · Jarek Poplawski
Quoted 3 lines · David Miller

From: Jarek Poplawski <[email protected]>
Date: Tue, 18 Jan 2011 09:38:11 +0000

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

Even if I'm wrong, this change added to stable will break many configs. My proposal is to revert commit 0ab03c2b147 until proper fix is found.

The flag combination is, at best ambiguous, it has no proper definition without the check we added.

Do you all expect all users manage to upgrade avahi app before changing their stable kernel? I mean "own distro" users especially.

The combination that avahi uses makes no sense.

I've been auditing user-space tools that may have problems with this change:

* iw (it uses libnl)
* acpid (it uses a mangled version of libnetlink shipped in iproute)
* tstime, for taskstats, it uses libnl
* wimax-tools, it uses libnl
* quota-tools, it uses libnl
* keepalived, no libs used

Well, I can keep looking for more, but I think that avahi is the only one doing this incorrectly.

Please, fix avahi instead.

maintainer
#16 lore raw

From: Pablo Neira Ayuso <[email protected]>
Date: Tue, 18 Jan 2011 21:31:31 +0100

Quoted 11 lines

The combination that avahi uses makes no sense.

I've been auditing user-space tools that may have problems with this change:

* iw (it uses libnl)
* acpid (it uses a mangled version of libnetlink shipped in iproute)
* tstime, for taskstats, it uses libnl
* wimax-tools, it uses libnl
* quota-tools, it uses libnl
* keepalived, no libs used

Quoted 4 lines

Well, I can keep looking for more, but I think that avahi is the only one doing this incorrectly.

Please, fix avahi instead.

That's a pretty compelling argument, so I'll hold off on the revert for now.

maintainer
#17 lore raw
Quoted 14 lines · David Miller

From: Pablo Neira Ayuso <[email protected]>
Date: Tue, 18 Jan 2011 21:31:31 +0100

Quoted 11 lines

The combination that avahi uses makes no sense.

I've been auditing user-space tools that may have problems with this change:

* iw (it uses libnl)
* acpid (it uses a mangled version of libnetlink shipped in iproute)
* tstime, for taskstats, it uses libnl
* wimax-tools, it uses libnl
* quota-tools, it uses libnl
* keepalived, no libs used

Quoted 7 lines
Quoted 4 lines

Well, I can keep looking for more, but I think that avahi is the only one doing this incorrectly.

Please, fix avahi instead.

That's a pretty compelling argument, so I'll hold off on the revert for now.

I've been reviewing user-space applications for a couple of hours (I've got a big list here with no problems), unfortunately I found that:

ip route show cache

hangs after displaying the first line with the patch applied because it uses:

        req.nlh.nlmsg_type = RTM_GETROUTE;
        req.nlh.nlmsg_flags = NLM_F_ROOT|NLM_F_REQUEST;

to dump the routing cache.

We need something less agressive, some warning to be printed and accept this flag combination for quite some time until it's removed as jamal and jarek suggested.

Please, revert this patch until we find a better solution.

maintainer
#18 lore raw

From: Pablo Neira Ayuso <[email protected]>
Date: Wed, 19 Jan 2011 18:42:13 +0100

Quoted 1 line

Please, revert this patch until we find a better solution.

Ok, done.

#19 lore raw
In reply to Pablo Neira Ayuso
Quoted 8 lines · Pablo Neira Ayuso
Quoted 7 lines · Jarek Poplawski
Quoted 6 lines · David Miller

From: Jarek Poplawski <[email protected]>
Date: Tue, 18 Jan 2011 09:38:11 +0000

Quoted 2 lines

Even if I'm wrong, this change added to stable will break many configs. My proposal is to revert commit 0ab03c2b147 until proper fix is found.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

The flag combination is, at best ambiguous, it has no proper definition without the check we added.

Do you all expect all users manage to upgrade avahi app before changing their stable kernel? I mean "own distro" users especially.

The combination that avahi uses makes no sense.

I don't agree as explained in the reverting patch. Anyway, again, this is an old problem, so no reason to force "fixing" it just now at the expense of the obvious regression especially in stable kernels Anyway, I'll accept any David's decision wrt this problem.

Jarek P.

#20 lore raw
Quoted 9 lines · Jarek Poplawski
Quoted 1 line · Pablo Neira Ayuso

The combination that avahi uses makes no sense.

I don't agree as explained in the reverting patch. Anyway, again, this is an old problem, so no reason to force "fixing" it just now at the expense of the obvious regression especially in stable kernels Anyway, I'll accept any David's decision wrt this problem.

So here is what i think the criteria should be:

If Avahi is popular and widely deployed (I dont use it anywhere), it makes no sense to revert.
A middle ground is: instead of rejecting the nonsense passed, maybe a sane thing to do is a kernel warning for a period of time (sort of like feature removal warnings).

The only way to keep the patch IMO (if avahi is widely deployed) is if common distro policy is such that they will immediately fix and distribute a new avahi even when this breakage is with a kernel that distro wont support for a year.

hope i am making sense.

cheers,
jamal

#21 lore raw
Quoted 11 lines · jamal
Quoted 9 lines · Jarek Poplawski
Quoted 1 line · Pablo Neira Ayuso

The combination that avahi uses makes no sense.

I don't agree as explained in the reverting patch. Anyway, again, this is an old problem, so no reason to force "fixing" it just now at the expense of the obvious regression especially in stable kernels Anyway, I'll accept any David's decision wrt this problem.

Quoted 7 lines

So here is what i think the criteria should be:

If Avahi is popular and widely deployed (I dont use it anywhere), it makes no sense to revert.
A middle ground is: instead of rejecting the nonsense passed, maybe a sane thing to do is a kernel warning for a period of time (sort of like feature removal warnings).

I still don't understand why you call this the nonsense. There are two dump flags NLM_F_ROOT and NLM_F_MATCH plus for convenience NLM_F_DUMP as 2 in 1. Avahi uses these specific flags. Why would anybody have added these specific flags if they can never be used separately?

Aside from this question, if we still think it's the nonsense, a warning would be nicer.

Cheers,
Jarek P.

#22 lore raw
Quoted 11 lines · Jarek Poplawski
Quoted 7 lines · jamal

So here is what i think the criteria should be:

If Avahi is popular and widely deployed (I dont use it anywhere), it makes no sense to revert.
A middle ground is: instead of rejecting the nonsense passed, maybe a sane thing to do is a kernel warning for a period of time (sort of like feature removal warnings).

I still don't understand why you call this the nonsense.

gah! I already had plenty of caffeine when i typed that. I meant to say "If Avahi is popular and widely deployed, it makes sense to revert"

Quoted 8 lines

There are
two dump flags NLM_F_ROOT and NLM_F_MATCH plus for convenience NLM_F_DUMP as 2 in 1. Avahi uses these specific flags. Why would anybody have added these specific flags if they can never be used separately?

Aside from this question, if we still think it's the nonsense, a warning would be nicer.

That is what i was suggesting as well..

cheers,
jamal

#23 lore raw
Quoted 1 line · jamal

On Wed, 2011-01-19 at 17:54 +0100, Jarek Poplawski wrote:

...

Quoted 3 lines

gah! I already had plenty of caffeine when i typed that. I meant to say "If Avahi is popular and widely deployed, it makes sense to revert"

AFAIK, the most popular distros (XP, Vista, W7) don't use Avahi! ;-) Other similar (desktop & userfriendly) should be affected.

Cheers,
Jarek P.

#24 lore raw
In reply to jamal
Quoted 1 line · jamal

On Wed, 2011-01-19 at 17:54 +0100, Jarek Poplawski wrote:

...

Quoted 4 lines
Quoted 2 lines

Aside from this question, if we still think it's the nonsense, a warning would be nicer.

That is what i was suggesting as well..

Except, I still don't think it's the nonsese, and suggest something even nicer ;-)

Cheers,
Jarek P.

#25 lore raw
In reply to Jarek Poplawski
Quoted 6 lines · Jarek Poplawski

I still don't understand why you call this the nonsense. There are two dump flags NLM_F_ROOT and NLM_F_MATCH plus for convenience NLM_F_DUMP as 2 in 1. Avahi uses these specific flags. Why would anybody have added these specific flags if they can never be used separately?

It looks like the authors' intentinos were to make NLM_F_MATCH not stop after a single entry has been found. So that sounds like dump, ok.

But NLM_F_ROOT does not quite strike me as a dump request. What if I wanted just a single item returned but still start at the root?

Or asking from a different direction, what's NLM_F_ROOT good for when, say, struct rtmsg->rtm_table specifies (in rtnetlink) where to start? (Particularly, 0 for an "invisible root" that contains all tables.)

#26 lore raw
Quoted 9 lines · Jan Engelhardt
Quoted 6 lines · Jarek Poplawski

I still don't understand why you call this the nonsense. There are two dump flags NLM_F_ROOT and NLM_F_MATCH plus for convenience NLM_F_DUMP as 2 in 1. Avahi uses these specific flags. Why would anybody have added these specific flags if they can never be used separately?

Quoted 6 lines

It looks like the authors' intentinos were to make NLM_F_MATCH not stop after a single entry has been found. So that sounds like dump, ok.

But NLM_F_ROOT does not quite strike me as a dump request. What if I wanted just a single item returned but still start at the root?

Hmm... Does it say about starting at the root?:

" NLM_F_ROOT Return the complete table instead of a
single entry."

Quoted 5 lines

Or asking from a different direction, what's NLM_F_ROOT good for when, say, struct rtmsg->rtm_table specifies (in rtnetlink) where to start? (Particularly, 0 for an "invisible root" that contains all tables.)

I can't say I understand these flags, but IMHO the main point is we should respect them as separate, even if mostly unused and look like unnecessary. (Unless there is really no other way of fixing this genetlink bug.) If it were undocumented... but after all this the RFC.

Jarek P.

#27 lore raw
Quoted 6 lines · Jarek Poplawski
Quoted 6 lines · Jarek Poplawski

It looks like the authors' intentinos were to make NLM_F_MATCH not stop after a single entry has been found. So that sounds like dump, ok.

Quoted 7 lines
Quoted 2 lines

But NLM_F_ROOT does not quite strike me as a dump request. What if I wanted just a single item returned but still start at the root?

Hmm... Does it say about starting at the root?:

" NLM_F_ROOT Return the complete table instead of a
single entry."

I was referring to netlink.h which paraphrased that, perhaps too short:

#define NLM_F_ROOT 0x100 /* specify tree root */

But the RFC description makes for a better wording: if NLM_F_ROOT is supposed to return "the complete table", how is it different from NLM_F_MATCH with a wildcard criteria?

|          NLM_F_MATCH    Return all entries matching criteria passed in
|                         message content.
#28 lore raw
Quoted 19 lines · Jan Engelhardt
Quoted 6 lines · Jarek Poplawski
Quoted 6 lines · Jarek Poplawski

It looks like the authors' intentinos were to make NLM_F_MATCH not stop after a single entry has been found. So that sounds like dump, ok.

Quoted 7 lines
Quoted 2 lines

But NLM_F_ROOT does not quite strike me as a dump request. What if I wanted just a single item returned but still start at the root?

Hmm... Does it say about starting at the root?:

" NLM_F_ROOT Return the complete table instead of a
single entry."

I was referring to netlink.h which paraphrased that, perhaps too short:

Quoted 8 lines

#define NLM_F_ROOT 0x100 /* specify tree root */

But the RFC description makes for a better wording: if NLM_F_ROOT is supposed to return "the complete table", how is it different from NLM_F_MATCH with a wildcard criteria?

|          NLM_F_MATCH    Return all entries matching criteria passed in
|                         message content.

As I said, I'd prefer not to pretend I understand it, but, knowing names of people around this, I'm also quite sure there was a purpose. On the other hand, I'm not sure the names of flags and descriptions weren't mixed while making it general for different subsystems.

BTW, don't we have in ip/tc many examples of duplicate options?

Jarek P.

#29 lore raw
In reply to Pablo Neira Ayuso
Quoted 25 lines · Pablo Neira Ayuso
Quoted 4 lines · Jarek Poplawski
Quoted 3 lines · David Miller

From: Jarek Poplawski <[email protected]>
Date: Tue, 18 Jan 2011 09:38:11 +0000

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

Even if I'm wrong, this change added to stable will break many configs. My proposal is to revert commit 0ab03c2b147 until proper fix is found.

The flag combination is, at best ambiguous, it has no proper definition without the check we added.

Do you all expect all users manage to upgrade avahi app before changing their stable kernel? I mean "own distro" users especially.

The combination that avahi uses makes no sense.

I've been auditing user-space tools that may have problems with this change:

* iw (it uses libnl)
* acpid (it uses a mangled version of libnetlink shipped in iproute)
* tstime, for taskstats, it uses libnl
* wimax-tools, it uses libnl
* quota-tools, it uses libnl
* keepalived, no libs used

Quoted 2 lines

Well, I can keep looking for more, but I think that avahi is the only one doing this incorrectly.

BTW, could you answer my earlier question, why NLM_F_ATOMIC flag isn't handled now with dumps?

Jarek P.

maintainer
#30 lore raw
Quoted 26 lines · Jarek Poplawski
Quoted 25 lines · Pablo Neira Ayuso
Quoted 4 lines · Jarek Poplawski
Quoted 3 lines · David Miller

From: Jarek Poplawski <[email protected]>
Date: Tue, 18 Jan 2011 09:38:11 +0000

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

Even if I'm wrong, this change added to stable will break many configs. My proposal is to revert commit 0ab03c2b147 until proper fix is found.

The flag combination is, at best ambiguous, it has no proper definition without the check we added.

Do you all expect all users manage to upgrade avahi app before changing their stable kernel? I mean "own distro" users especially.

The combination that avahi uses makes no sense.

I've been auditing user-space tools that may have problems with this change:

* iw (it uses libnl)
* acpid (it uses a mangled version of libnetlink shipped in iproute)
* tstime, for taskstats, it uses libnl
* wimax-tools, it uses libnl
* quota-tools, it uses libnl
* keepalived, no libs used

Quoted 5 lines
Quoted 2 lines

Well, I can keep looking for more, but I think that avahi is the only one doing this incorrectly.

BTW, could you answer my earlier question, why NLM_F_ATOMIC flag isn't handled now with dumps?

The NLM_F_ATOMIC flag is not affected, the netlink header is still passed to netlink_dump_start() so you can check for it in the callback to start an atomic dump.

#31 lore raw
Quoted 1 line · Pablo Neira Ayuso

On 18/01/11 22:14, Jarek Poplawski wrote:

...

Quoted 6 lines
Quoted 2 lines

BTW, could you answer my earlier question, why NLM_F_ATOMIC flag isn't handled now with dumps?

The NLM_F_ATOMIC flag is not affected, the netlink header is still passed to netlink_dump_start() so you can check for it in the callback to start an atomic dump.

Right! I had some blackout, sorry.

Jarek P.