From: Wang Chen <wangchen@cn•fujitsu.com>
To: Patrick McHardy <kaber@trash•net>
Cc: "David S. Miller" <davem@davemloft•net>, NETDEV <netdev@vger•kernel.org>
Subject: Re: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI
Date: Tue, 17 Jun 2008 09:42:57 +0800 [thread overview]
Message-ID: <485716A1.8030401@cn.fujitsu.com> (raw)
In-Reply-To: <48563A7F.50309@trash.net>
Patrick McHardy said the following on 2008-6-16 18:03:
> Wang Chen wrote:
>> Patrick McHardy said the following on 2008-6-16 17:27:
>>> Wang Chen wrote:
>>>> - if (dev->flags & IFF_ALLMULTI)
>>>> - dev_set_allmulti(real_dev, 1);
>>>> + /* NOTE: order of synchronization of IFF_PROMISC and IFF_ALLMULTI
>>>> + is important. Some (broken) drivers set IFF_PROMISC, when
>>>> + IFF_ALLMULTI is requested not asking us and not reporting.
>>>> + */
>>>> if (dev->flags & IFF_PROMISC)
>>>> dev_set_promiscuity(real_dev, 1);
>>>> + if (dev->flags & IFF_ALLMULTI)
>>>> + dev_set_allmulti(real_dev, 1);
>>>
>>> What exactly is the problem here? The VLAN code is obviously not
>>> one of the broken drivers, so why should it care what other drivers
>>> do?
>>>
>>
>> I think the problem is that allmulti is not valid if promis is not on.
>
> No, PROMISC is a superset of ALLMULTI.
>
>> And about the comment, I copy it from dev_change_flags() and think
>> it seems suit for here.
>> Did I misunderstand this comment?
>
> I think it refers to broken behaviour by drivers that set
> IFF_PROMISC themselves when asked to disable multicast
> filtering by setting IFF_ALLMULTI. This would cause the
> test for changed flags in dev_set_promiscuity to return zero
> and not program the device for promiscous mode properly.
>
Do you mean things like that in do_mc32_set_multicast_list()?
> There are a few examples of this in the tree. But calling
> dev_set_promiscuity() before dev_set_allmulti() only helps
> in the dev_change_flags() case since its the only function
> that might change both flags at once. In all other cases it
> depends on the caller.
>
> So for the dev_change_flags() case VLAN already uses the
> "proper" ordering, the other cases might be broken with
> or without your patch.
>
Is there any other case might be broken?
> I'd suggest to fix the drivers instead, perhaps start by
> adding a warning to dev_change_flags() that is triggered
> by the driver changing the flags itself.
>
In some driver's code of *_set_multicast_list(), IFF_PROMISC
will be set if IFF_ALLMULTI is set.
And there is comment about the necessity for setting IFF_PROMISC.
/*
* We must make the kernel realise we had to move
* into promisc mode or we start all out war on
* the cable. If it was a promisc request the
* flag is already set. If not we assert it.
*/
So, I doubt about fixing the drivers.
next prev parent reply other threads:[~2008-06-17 1:46 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-06-16 9:17 [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI Wang Chen
2008-06-16 9:27 ` Patrick McHardy
2008-06-16 9:39 ` Wang Chen
2008-06-16 10:03 ` Patrick McHardy
2008-06-17 1:42 ` Wang Chen [this message]
2008-06-17 13:06 ` Patrick McHardy
2008-06-18 2:27 ` Wang Chen
2008-06-18 2:52 ` Jeff Garzik
2008-06-18 3:22 ` Wang Chen
2008-06-20 15:07 ` [PATCH] net-driver: Drivers don't set IFF_* flag [Was: [PATCH 3/3] netdevice: order of synchronization of IFF_PROMISC and IFF_ALLMULTI] Wang Chen
2008-06-23 11:04 ` Patrick McHardy
2008-06-23 13:33 ` Wang Chen
2008-06-23 13:47 ` Patrick McHardy
2008-06-23 14:44 ` Wang Chen
2008-06-23 14:52 ` Patrick McHardy
2008-06-24 1:02 ` Wang Chen
2008-06-24 5:10 ` Grant Grundler
2008-06-24 5:39 ` Wang Chen
2008-06-27 1:14 ` v2 [PATCH 2/2] de4x5: Remove developer debug feature about set/clear promisc Wang Chen
2008-06-28 17:52 ` Grant Grundler
2008-06-30 3:24 ` v3 " Wang Chen
2008-07-02 4:22 ` Grant Grundler
2008-06-27 1:14 ` v2 [PATCH 1/2] net-driver: Drivers don't set IFF_* flag Wang Chen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=485716A1.8030401@cn.fujitsu.com \
--to=wangchen@cn$(echo .)fujitsu.com \
--cc=davem@davemloft$(echo .)net \
--cc=kaber@trash$(echo .)net \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox