diff mbox

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

Message ID 4D336050.9030602@netfilter.org
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Pablo Neira Ayuso Jan. 16, 2011, 9:17 p.m. UTC 
On 16/01/11 13:25, Arthur Marsh wrote:
> Jan Engelhardt wrote, on 16/01/11 21:20:
>>
>> Le dimanche 16 janvier 2011 à 19:24 +1030, Arthur Marsh a écrit :
>>>
>>>> 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.
> 
>>
>>>> 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.
> 
> 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.

Comments

Arthur Marsh Jan. 17, 2011, 1:03 a.m. UTC | #1 
Pablo Neira Ayuso wrote, on 17/01/11 07:47:
> On 16/01/11 13:25, Arthur Marsh wrote:
>> Jan Engelhardt wrote, on 16/01/11 21:20:
>>>
>>> Le dimanche 16 janvier 2011 à 19:24 +1030, Arthur Marsh a écrit :
>>>>
>>>>> 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.
>>
>>>
>>>>> 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.
>>
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarek Poplawski Jan. 18, 2011, 9:38 a.m. UTC | #2 
On 2011-01-16 22:17, Pablo Neira Ayuso wrote:
> On 16/01/11 13:25, Arthur Marsh wrote:
>> Jan Engelhardt wrote, on 16/01/11 21:20:
>>>
>>> Le dimanche 16 janvier 2011 Ă  19:24 +1030, Arthur Marsh a ĂŠcrit :
>>>>
>>>>> 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.
>>
>>>
>>>>> 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.
>>
>> 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 <hadi@cyberus.ca>

Jarek P.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Jan. 18, 2011, 10:07 a.m. UTC | #3 
From: Jarek Poplawski <jarkao2@gmail.com>
Date: Tue, 18 Jan 2011 09:38:11 +0000

> 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarek Poplawski Jan. 18, 2011, 10:24 a.m. UTC | #4 
On Tue, Jan 18, 2011 at 02:07:02AM -0800, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Tue, 18 Jan 2011 09:38:11 +0000
> 
> > 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
jamal Jan. 18, 2011, 2:05 p.m. UTC | #5 
On Tue, 2011-01-18 at 10:24 +0000, Jarek Poplawski wrote:

> 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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
jamal Jan. 18, 2011, 2:07 p.m. UTC | #6 
On Tue, 2011-01-18 at 09:05 -0500, jamal wrote:
> On Tue, 2011-01-18 at 10:24 +0000, Jarek Poplawski wrote:
> 
> > 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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarek Poplawski Jan. 18, 2011, 6:11 p.m. UTC | #7 
On Tue, Jan 18, 2011 at 09:05:03AM -0500, jamal wrote:
> On Tue, 2011-01-18 at 10:24 +0000, Jarek Poplawski wrote:
> 
> > 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.

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.

> 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso Jan. 18, 2011, 8:31 p.m. UTC | #8 
On 18/01/11 11:24, Jarek Poplawski wrote:
> On Tue, Jan 18, 2011 at 02:07:02AM -0800, David Miller wrote:
>> From: Jarek Poplawski <jarkao2@gmail.com>
>> Date: Tue, 18 Jan 2011 09:38:11 +0000
>>
>>> 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Jan. 18, 2011, 8:39 p.m. UTC | #9 
From: jamal <hadi@cyberus.ca>
Date: Tue, 18 Jan 2011 09:05:03 -0500

> On Tue, 2011-01-18 at 10:24 +0000, Jarek Poplawski wrote:
> 
>> 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.

Fair enough, I'll revert the netlink patch.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Jan. 18, 2011, 8:50 p.m. UTC | #10 
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Tue, 18 Jan 2011 21:31:31 +0100

> 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.

That's a pretty compelling argument, so I'll hold off on the revert
for now.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarek Poplawski Jan. 18, 2011, 8:55 p.m. UTC | #11 
On Tue, Jan 18, 2011 at 09:31:31PM +0100, Pablo Neira Ayuso wrote:
> On 18/01/11 11:24, Jarek Poplawski wrote:
> > On Tue, Jan 18, 2011 at 02:07:02AM -0800, David Miller wrote:
> >> From: Jarek Poplawski <jarkao2@gmail.com>
> >> Date: Tue, 18 Jan 2011 09:38:11 +0000
> >>
> >>> 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 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarek Poplawski Jan. 18, 2011, 9:14 p.m. UTC | #12 
On Tue, Jan 18, 2011 at 09:31:31PM +0100, Pablo Neira Ayuso wrote:
> On 18/01/11 11:24, Jarek Poplawski wrote:
> > On Tue, Jan 18, 2011 at 02:07:02AM -0800, David Miller wrote:
> >> From: Jarek Poplawski <jarkao2@gmail.com>
> >> Date: Tue, 18 Jan 2011 09:38:11 +0000
> >>
> >>> 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.

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

Jarek P.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
jamal Jan. 19, 2011, 2:28 p.m. UTC | #13 
On Tue, 2011-01-18 at 21:55 +0100, Jarek Poplawski wrote:
> On Tue, Jan 18, 2011 at 09:31:31PM +0100, Pablo Neira Ayuso wrote:

> > 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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso Jan. 19, 2011, 2:53 p.m. UTC | #14 
On 18/01/11 22:14, Jarek Poplawski wrote:
> On Tue, Jan 18, 2011 at 09:31:31PM +0100, Pablo Neira Ayuso wrote:
>> On 18/01/11 11:24, Jarek Poplawski wrote:
>>> On Tue, Jan 18, 2011 at 02:07:02AM -0800, David Miller wrote:
>>>> From: Jarek Poplawski <jarkao2@gmail.com>
>>>> Date: Tue, 18 Jan 2011 09:38:11 +0000
>>>>
>>>>> 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.
> 
> 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarek Poplawski Jan. 19, 2011, 4:18 p.m. UTC | #15 
On Wed, Jan 19, 2011 at 03:53:17PM +0100, Pablo Neira Ayuso wrote:
> On 18/01/11 22:14, Jarek Poplawski wrote:
...
> > 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarek Poplawski Jan. 19, 2011, 4:54 p.m. UTC | #16 
On Wed, Jan 19, 2011 at 09:28:06AM -0500, jamal wrote:
> On Tue, 2011-01-18 at 21:55 +0100, Jarek Poplawski wrote:
> > On Tue, Jan 18, 2011 at 09:31:31PM +0100, Pablo Neira Ayuso wrote:
> 
> > > 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).

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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
jamal Jan. 19, 2011, 4:59 p.m. UTC | #17 
On Wed, 2011-01-19 at 17:54 +0100, Jarek Poplawski wrote:
> On Wed, Jan 19, 2011 at 09:28:06AM -0500, jamal wrote:

> > 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"

> 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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarek Poplawski Jan. 19, 2011, 5:19 p.m. UTC | #18 
On Wed, Jan 19, 2011 at 11:59:37AM -0500, jamal wrote:
> On Wed, 2011-01-19 at 17:54 +0100, Jarek Poplawski wrote:
...
> 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarek Poplawski Jan. 19, 2011, 5:33 p.m. UTC | #19 
On Wed, Jan 19, 2011 at 11:59:37AM -0500, jamal wrote:
> On Wed, 2011-01-19 at 17:54 +0100, Jarek Poplawski wrote:
...
> > 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso Jan. 19, 2011, 5:42 p.m. UTC | #20 
On 18/01/11 21:50, David Miller wrote:
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Date: Tue, 18 Jan 2011 21:31:31 +0100
> 
>> 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.
> 
> 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Engelhardt Jan. 19, 2011, 6:04 p.m. UTC | #21 
On Wednesday 2011-01-19 17:54, Jarek Poplawski wrote:
>
>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.)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarek Poplawski Jan. 19, 2011, 7:24 p.m. UTC | #22 
On Wed, Jan 19, 2011 at 07:04:06PM +0100, Jan Engelhardt wrote:
> 
> On Wednesday 2011-01-19 17:54, Jarek Poplawski wrote:
> >
> >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?

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

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

> 
> 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Engelhardt Jan. 19, 2011, 7:47 p.m. UTC | #23 
On Wednesday 2011-01-19 20:24, Jarek Poplawski wrote:
>> On Wednesday 2011-01-19 17:54, Jarek Poplawski wrote:
>> 
>> 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."

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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarek Poplawski Jan. 19, 2011, 8:12 p.m. UTC | #24 
On Wed, Jan 19, 2011 at 08:47:32PM +0100, Jan Engelhardt wrote:
> 
> On Wednesday 2011-01-19 20:24, Jarek Poplawski wrote:
> >> On Wednesday 2011-01-19 17:54, Jarek Poplawski wrote:
> >> 
> >> 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."
> 
> 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.

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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Jan. 19, 2011, 9:34 p.m. UTC | #25 
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Wed, 19 Jan 2011 18:42:13 +0100

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

Ok, done.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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 <pablo@netfilter.org>

Index: avahi-0.6.27/avahi-autoipd/iface-linux.c
===================================================================
--- avahi-0.6.27.orig/avahi-autoipd/iface-linux.c	2011-01-16 22:06:20.000000000 +0100
+++ avahi-0.6.27/avahi-autoipd/iface-linux.c	2011-01-16 22:07:34.000000000 +0100
@@ -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);
Index: avahi-0.6.27/avahi-core/iface-linux.c
===================================================================
--- avahi-0.6.27.orig/avahi-core/iface-linux.c	2011-01-16 22:06:51.000000000 +0100
+++ avahi-0.6.27/avahi-core/iface-linux.c	2011-01-16 22:07:08.000000000 +0100
@@ -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);