* [PATCH net-next v2] rhashtable: rhashtable_remove() must unlink in both tbl and future_tbl
From: Thomas Graf @ 2015-01-21 11:54 UTC (permalink / raw)
To: davem, Sergei Shtylyov
Cc: Ying Xue, richard.alpe@ericsson.com >> Richard Alpe, Netdev,
tipc-discussion
In-Reply-To: <54BEA1F9.1050908@cogentembedded.com>
As removals can occur during resizes, entries may be referred to from
both tbl and future_tbl when the removal is requested. Therefore
rhashtable_remove() must unlink the entry in both tables if this is
the case. The existing code did search both tables but stopped when it
hit the first match.
Failing to unlink in both tables resulted in use after free.
Fixes: 97defe1ecf86 ("rhashtable: Per bucket locks & deferred expansion/shrinking")
Reported-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
v2: 12 digit commit reference, commit msg clarification as pointed
out by Sergei.
lib/rhashtable.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 84a78e3..bc2d0d8 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -585,6 +585,7 @@ bool rhashtable_remove(struct rhashtable *ht, struct rhash_head *obj)
struct rhash_head *he;
spinlock_t *lock;
unsigned int hash;
+ bool ret = false;
rcu_read_lock();
tbl = rht_dereference_rcu(ht->tbl, ht);
@@ -602,17 +603,16 @@ restart:
}
rcu_assign_pointer(*pprev, obj->next);
- atomic_dec(&ht->nelems);
-
- spin_unlock_bh(lock);
-
- rhashtable_wakeup_worker(ht);
-
- rcu_read_unlock();
- return true;
+ ret = true;
+ break;
}
+ /* The entry may be linked in either 'tbl', 'future_tbl', or both.
+ * 'future_tbl' only exists for a short period of time during
+ * resizing. Thus traversing both is fine and the added cost is
+ * very rare.
+ */
if (tbl != rht_dereference_rcu(ht->future_tbl, ht)) {
spin_unlock_bh(lock);
@@ -625,9 +625,15 @@ restart:
}
spin_unlock_bh(lock);
+
+ if (ret) {
+ atomic_dec(&ht->nelems);
+ rhashtable_wakeup_worker(ht);
+ }
+
rcu_read_unlock();
- return false;
+ return ret;
}
EXPORT_SYMBOL_GPL(rhashtable_remove);
--
1.9.3
^ permalink raw reply related
* Re: [PATCH v5 2/5] can: kvaser_usb: Consolidate and unify state change handling
From: Wolfgang Grandegger @ 2015-01-21 11:53 UTC (permalink / raw)
To: Andri Yngvason
Cc: Ahmed S. Darwish, Olivier Sobrie, Oliver Hartkopp,
Marc Kleine-Budde, Linux-CAN, netdev, LKML
In-Reply-To: <20150121103319.14511.57709@shannon>
On Wed, 21 Jan 2015 10:33:19 +0000, Andri Yngvason
<andri.yngvason@marel.com> wrote:
> Quoting Ahmed S. Darwish (2015-01-20 21:45:37)
>> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
>>
>> Replace most of the can interface's state and error counters
>> handling with the new can-dev can_change_state() mechanism.
>>
>> Suggested-by: Andri Yngvason <andri.yngvason@marel.com>
>> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
>> ---
>> drivers/net/can/usb/kvaser_usb.c | 114
>> +++++++++++++++++++--------------------
>> 1 file changed, 55 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/net/can/usb/kvaser_usb.c
>> b/drivers/net/can/usb/kvaser_usb.c
>> index 971c5f9..0386d3f 100644
>> --- a/drivers/net/can/usb/kvaser_usb.c
>> +++ b/drivers/net/can/usb/kvaser_usb.c
...
>
> Looks good.
Would be nice to see some "candump" traces as well. Ahmed, could you
please generate such traces doing:
1. Execute in a session:
# candump -t d -e any,0:0,#FFFFFFFF
2. Execute in another session:
# cangen -g 10 -D i can0
3. Disconnect the CAN cable
4. After a while reconnect the CAN cable
5. Stop candump and save the trace.
and to test bus-off:
1. Execute in a session:
# candump -t d -e any,0:0,#FFFFFFFF
2. Execute in another session:
# cangen -g 10 -D i can0
3. Short-circuit the CAN low and high wires of the CAN cable
4. After a while remove the short-circuit.
5. Stop candump and save the trace.
Thanks,
Wolfgang.
^ permalink raw reply
* [PATCH v2] net: wireless: atmel: Remove open-coded and wrong strcasecmp
From: Rasmus Villemoes @ 2015-01-21 11:52 UTC (permalink / raw)
To: Simon Kelley, Kalle Valo
Cc: Sergei Shtylyov, Rasmus Villemoes, linux-wireless, netdev,
linux-kernel
In-Reply-To: <1421414907-26764-1-git-send-email-linux@rasmusvillemoes.dk>
The kernel's string library does in fact have strcasecmp, at least
since ded220bd8f08 ("[STRING]: Move strcasecmp/strncasecmp to
lib/string.c"). Moreover, this open-coded version is in fact wrong: If
the strings only differ in their last character, a and b have already
been incremented to point to the terminating NUL bytes, so they would
wrongly be treated as equal.
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
v2: Address Sergei's comments to the commit message.
drivers/net/wireless/atmel.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/drivers/net/wireless/atmel.c b/drivers/net/wireless/atmel.c
index 9183f1cf89a7..55db9f03eb2a 100644
--- a/drivers/net/wireless/atmel.c
+++ b/drivers/net/wireless/atmel.c
@@ -45,7 +45,6 @@
#include <linux/ptrace.h>
#include <linux/slab.h>
#include <linux/string.h>
-#include <linux/ctype.h>
#include <linux/timer.h>
#include <asm/byteorder.h>
#include <asm/io.h>
@@ -2699,16 +2698,7 @@ static int atmel_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
domain[REGDOMAINSZ] = 0;
rc = -EINVAL;
for (i = 0; i < ARRAY_SIZE(channel_table); i++) {
- /* strcasecmp doesn't exist in the library */
- char *a = channel_table[i].name;
- char *b = domain;
- while (*a) {
- char c1 = *a++;
- char c2 = *b++;
- if (tolower(c1) != tolower(c2))
- break;
- }
- if (!*a && !*b) {
+ if (!strcasecmp(channel_table[i].name, domain)) {
priv->config_reg_domain = channel_table[i].reg_domain;
rc = 0;
}
--
2.1.3
^ permalink raw reply related
* [PATCH net] ipv6: tcp: fix race in IPV6_2292PKTOPTIONS
From: Eric Dumazet @ 2015-01-21 11:45 UTC (permalink / raw)
To: David Miller; +Cc: netdev
From: Eric Dumazet <edumazet@google.com>
IPv6 TCP sockets store in np->pktoptions skbs, and use skb_set_owner_r()
to charge the skb to socket.
It means that destructor must be called while socket is locked.
Therefore, we cannot use skb_get() or atomic_inc(&skb->users)
to protect ourselves : kfree_skb() might race with other users
manipulating sk->sk_forward_alloc
Fix this race by holding socket lock for the duration of
ip6_datagram_recv_ctl()
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
Since this bug is very old, feel free to apply on net-next ;)
net/ipv6/ipv6_sockglue.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 66980d8d98d1f5b3ef7a50dc33cb9b617f25604d..8d766d9100cba408525faf5818b7b0c6b6bc543c 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -996,13 +996,9 @@ static int do_ipv6_getsockopt(struct sock *sk, int level, int optname,
lock_sock(sk);
skb = np->pktoptions;
if (skb)
- atomic_inc(&skb->users);
- release_sock(sk);
-
- if (skb) {
ip6_datagram_recv_ctl(sk, &msg, skb);
- kfree_skb(skb);
- } else {
+ release_sock(sk);
+ if (!skb) {
if (np->rxopt.bits.rxinfo) {
struct in6_pktinfo src_info;
src_info.ipi6_ifindex = np->mcast_oif ? np->mcast_oif :
^ permalink raw reply related
* Re: [PATCH] rhashtable: fix rht_for_each_entry_safe() endless loop
From: Thomas Graf @ 2015-01-21 11:43 UTC (permalink / raw)
To: Patrick McHardy; +Cc: davem, netdev
In-Reply-To: <1421838733-16495-1-git-send-email-kaber@trash.net>
On 01/21/15 at 11:12am, Patrick McHardy wrote:
> "next" is not updated, causing an endless loop for buckets with more than
> one element.
>
> Signed-off-by: Patrick McHardy <kaber@trash.net>
Fixes: 88d6ed15acff ("rhashtable: Convert bucket iterators to take table and index")
Acked-by: Thomas Graf <tgraf@suug.ch>
Dave: This only affects net-next.
^ permalink raw reply
* Re: [PATCH 1/2] if_link: Add VF multicast promiscuous mode control
From: Hiroshi Shimamoto @ 2015-01-21 11:38 UTC (permalink / raw)
To: Skidmore, Donald C, Bjørn Mork
Cc: e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org,
Choi, Sy Jong, Hayato Momma, linux-kernel@vger.kernel.org
In-Reply-To: <F6FB0E698C9B3143BDF729DF222866469129B844@ORSMSX110.amr.corp.intel.com>
> Subject: RE: [E1000-devel] [PATCH 1/2] if_link: Add VF multicast promiscuous mode control
>
>
>
> > -----Original Message-----
> > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com]
> > Sent: Tuesday, January 20, 2015 5:07 PM
> > To: Skidmore, Donald C; Bjørn Mork
> > Cc: e1000-devel@lists.sourceforge.net; netdev@vger.kernel.org; Choi, Sy
> > Jong; linux-kernel@vger.kernel.org; Hayato Momma
> > Subject: RE: [E1000-devel] [PATCH 1/2] if_link: Add VF multicast promiscuous
> > mode control
> >
> > > Subject: RE: [E1000-devel] [PATCH 1/2] if_link: Add VF multicast
> > > promiscuous mode control
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Hiroshi Shimamoto [mailto:h-shimamoto@ct.jp.nec.com]
> > > > Sent: Tuesday, January 20, 2015 3:40 PM
> > > > To: Bjørn Mork
> > > > Cc: e1000-devel@lists.sourceforge.net; netdev@vger.kernel.org; Choi,
> > > > Sy Jong; linux-kernel@vger.kernel.org; Hayato Momma
> > > > Subject: Re: [E1000-devel] [PATCH 1/2] if_link: Add VF multicast
> > > > promiscuous mode control
> > > >
> > > > > Subject: Re: [PATCH 1/2] if_link: Add VF multicast promiscuous
> > > > > mode control
> > > > >
> > > > > Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> writes:
> > > > >
> > > > > > From: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> > > > > >
> > > > > > Add netlink directives and ndo entry to control VF multicast
> > > > > > promiscuous
> > > > mode.
> > > > > >
> > > > > > Intel ixgbe and ixgbevf driver can handle only 30 multicast MAC
> > > > > > addresses per VF. It means that we cannot assign over 30 IPv6
> > > > > > addresses to a single VF interface on VM. We want thousands IPv6
> > > > addresses in VM.
> > > > > >
> > > > > > There is capability of multicast promiscuous mode in Intel 82599 chip.
> > > > > > It enables all multicast packets are delivered to the target VF.
> > > > > >
> > > > > > This patch prepares to control that VF multicast promiscuous
> > > > functionality.
> > > > >
> > > > > Adding a new hook for this seems over-complicated to me. And it
> > > > > still doesn't solve the real problems that
> > > > > a) the user has to know about this limit, and
> > > > > b) manually configure the feature
> > > > >
> > > > > Most of us, lacking the ability to imagine such arbitrary hardware
> > > > > limitations, will go through a few hours of frustrating debugging
> > > > > before we figure this one out...
> > > > >
> > > > > Why can't the ixgbevf driver just automatically signal the ixgbe
> > > > > driver to enable multicast promiscuous mode whenever the list
> > > > > grows past the limit?
> > > >
> > > > I had submitted a patch to change ixgbe and ixgbevf driver for this issue.
> > > > https://lkml.org/lkml/2014/11/27/269
> > > >
> > > > The previous patch introduces API between ixgbe and ixgbevf driver
> > > > to enable multicast promiscuous mode, and ixgbevf enables it
> > > > automatically if the number of addresses is over than 30.
> > >
> > > I believe the issue is with allowing a VF to automatically enter
> > > Promiscuous Multicast without the PF's ok is concern
> >
> > So you mean that we should take care about enabling VF multicast
> > promiscuous mode in host side, right? The host allows multicast promiscuous
> > and VF requests it too, then enables VF multicast promiscuous mode.
> > So, what is preferred way to do in host do you think?
>
> I think we are saying the same thing. I believe it would be fine if the VF requests this to happen (threw a mailbox message
> like you set up) and the PF will do it if the systems policy has been set up that way (as you did with the control mode).
>
> This way the behavior (related to multicast) is the same as it has been, unless the system has been setup specifically
> to allow VF multicast promiscuous mode.
Now I understand what you're saying.
Will make patches that make knob in host/PF and ixgbe/ixgbevf interface.
I think I should try to find whether there is a way to make the know without ndo.
>
> I know it's been mentioned that this is onerous on those who want this behavior to be automatic, but I don't see how else
> it could be done and take account for people that are concerned about allowing a (possibly untrusted) VM promoting itself
> in to multicast promiscuous mode.
I didn't mind its onerousness so much.
My concern is what is the real issue that VF multicast promiscuous mode can cause.
I think there is the 4k entries to filter multicast address, and the current ixgbe/ixgbevf
can turn all bits on from VM. That is almost same as enabling multicast promiscuous mode.
I mean that we can receive all multicast addresses by an onerous operation in untrusted VM.
I think we should clarify what is real security issue in this context.
thanks,
Hiroshi
>
> >
> > > over VM isolation. Of course that isolation, when it comes to multicast, is
> > rather limited anyway given that our multicast
> > > filter uses only 12-bit of the address for a match. Still this (or
> > > doing it by default) would only open that up considerably more (all
> > > multicasts). I assume for your application you're not concerned, but are
> > there other use cases that would worry about such things?
> >
> > Sorry I couldn't catch the point.
> >
> > What is the issue? I think there is no difference for the users who don't want
> > many multicast addresses in guest. In the current implementation,
> > overflowed multicast addresses silently discarded in ixgbevf. I believe there
> > is no user who want to use over 30 multicast addresses now. If VF multicast
> > promiscuous mode is enabled in certain VF, the behavior of other VFs is not
> > changed.
> >
> > thanks,
> > Hiroshi
> >
> > >
> > > Thanks,
> > > -Don Skidmore <donald.c.skidmore@intel.com>
> > >
> > > >
> > > > I got some comment and I would like to clarify the point, but there
> > > > was no answer.
> > > > That's the reason I submitted this patch.
> > > >
> > > > Do you think a patch for the ixgbe/ixgbevf driver is preferred?
> > > >
> > > >
> > > > thanks,
> > > > Hiroshi
> > > >
> > > > >
> > > > > I'd also like to note that this comment in
> > > > > drivers/net/ethernet/intel/ixgbevf/vf.c
> > > > > indicates that the author had some ideas about how more than 30
> > > > > addresses could/should be handled:
> > > > >
> > > > > static s32 ixgbevf_update_mc_addr_list_vf(struct ixgbe_hw *hw,
> > > > > struct net_device *netdev)
> > > > > {
> > > > > struct netdev_hw_addr *ha;
> > > > > u32 msgbuf[IXGBE_VFMAILBOX_SIZE];
> > > > > u16 *vector_list = (u16 *)&msgbuf[1];
> > > > > u32 cnt, i;
> > > > >
> > > > > /* Each entry in the list uses 1 16 bit word. We have 30
> > > > > * 16 bit words available in our HW msg buffer (minus 1 for the
> > > > > * msg type). That's 30 hash values if we pack 'em right. If
> > > > > * there are more than 30 MC addresses to add then punt the
> > > > > * extras for now and then add code to handle more than 30 later.
> > > > > * It would be unusual for a server to request that many multi-cast
> > > > > * addresses except for in large enterprise network environments.
> > > > > */
> > > > >
> > > > >
> > > > >
> > > > > The last 2 lines of that comment are of course totally bogus and
> > > > > pointless and should be deleted in any case... It's obvious that
> > > > > 30 multicast addresses is ridiculously low for lots of normal use cases.
> > > > >
> > > > >
> > > > > Bjørn
------------------------------------------------------------------------------
New Year. New Location. New Benefits. New Data Center in Ashburn, VA.
GigeNET is offering a free month of service with a new server in Ashburn.
Choose from 2 high performing configs, both with 100TB of bandwidth.
Higher redundancy.Lower latency.Increased capacity.Completely compliant.
http://p.sf.net/sfu/gigenet
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Thomas Graf @ 2015-01-21 11:37 UTC (permalink / raw)
To: Patrick McHardy
Cc: Herbert Xu, davem, paulmck, ying.xue, netdev, netfilter-devel
In-Reply-To: <20150121103959.GR3012@acer.localdomain>
On 01/21/15 at 10:40am, Patrick McHardy wrote:
> On the danger of repeating myself, every (converted) user requires
> that we at least keep the existing semantics since it is exposed to
> userspace. My opinion is that NLM_F_DUMP_INTR is fine if userspace
> indicates support, but without that, we have to take care of that
> in the kernel.
Absolutely agreed. I think this is an excellent low cost path for
future users where dumps are rare.
> An automatic restart handles this well. Userspace always had to
> expect duplicates.
Maybe I don't understand the restart yet. How do you restart if the
dump was already started and the user has read part of the dump?
^ permalink raw reply
* [PATCH] rhashtable: fix rht_for_each_entry_safe() endless loop
From: Patrick McHardy @ 2015-01-21 11:12 UTC (permalink / raw)
To: tgraf; +Cc: davem, netdev, Patrick McHardy
"next" is not updated, causing an endless loop for buckets with more than
one element.
Signed-off-by: Patrick McHardy <kaber@trash.net>
---
include/linux/rhashtable.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index 9570832..83944f9 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h
@@ -260,7 +260,9 @@ void rhashtable_destroy(struct rhashtable *ht);
next = !rht_is_a_nulls(pos) ? \
rht_dereference_bucket(pos->next, tbl, hash) : NULL; \
(!rht_is_a_nulls(pos)) && rht_entry(tpos, pos, member); \
- pos = next)
+ pos = next, \
+ next = !rht_is_a_nulls(pos) ? \
+ rht_dereference_bucket(pos->next, tbl, hash) : NULL)
/**
* rht_for_each_rcu_continue - continue iterating over rcu hash chain
--
2.1.0
^ permalink raw reply related
* 2015 OFFER
From: CHEVROLET CAR COMPANY @ 2015-01-21 9:51 UTC (permalink / raw)
YOUR EMAIL ID HAVE WON 3 CRORES 35LAHKS INR & A CAR FROM CHEVROLET CAR
COMPANY. TO CLAIM SEND YOUR EMAIL ID/NAME/PHONE NO/AGE/ADDRESS/OCCUPATION
TO cvclaim2@gmail.com
^ permalink raw reply
* Re: [PATCH v5 2/5] can: kvaser_usb: Consolidate and unify state change handling
From: Andri Yngvason @ 2015-01-21 11:00 UTC (permalink / raw)
To: Marc Kleine-Budde, Ahmed S. Darwish, Olivier Sobrie,
Oliver Hartkopp, Wolfgang Grandegger
Cc: Linux-CAN, netdev, LKML
In-Reply-To: <54BF8326.3080406@pengutronix.de>
Quoting Marc Kleine-Budde (2015-01-21 10:44:54)
> On 01/21/2015 11:33 AM, Andri Yngvason wrote:
> > Quoting Ahmed S. Darwish (2015-01-20 21:45:37)
> >> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> >>
> >> Replace most of the can interface's state and error counters
> >> handling with the new can-dev can_change_state() mechanism.
> >>
> >> Suggested-by: Andri Yngvason <andri.yngvason@marel.com>
> >> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> >> ---
> >> drivers/net/can/usb/kvaser_usb.c | 114 +++++++++++++++++++--------------------
> >> 1 file changed, 55 insertions(+), 59 deletions(-)
> >>
> >> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> >> index 971c5f9..0386d3f 100644
> >> --- a/drivers/net/can/usb/kvaser_usb.c
> >> +++ b/drivers/net/can/usb/kvaser_usb.c
>
> > Looks good.
>
> Is this an Acked-by?
>
ACK.
--
Andri
^ permalink raw reply
* Re: [PATCH v5 2/5] can: kvaser_usb: Consolidate and unify state change handling
From: Marc Kleine-Budde @ 2015-01-21 10:44 UTC (permalink / raw)
To: Andri Yngvason, Ahmed S. Darwish, Olivier Sobrie, Oliver Hartkopp,
Wolfgang Grandegger
Cc: Linux-CAN, netdev, LKML
In-Reply-To: <20150121103319.14511.57709@shannon>
[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]
On 01/21/2015 11:33 AM, Andri Yngvason wrote:
> Quoting Ahmed S. Darwish (2015-01-20 21:45:37)
>> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
>>
>> Replace most of the can interface's state and error counters
>> handling with the new can-dev can_change_state() mechanism.
>>
>> Suggested-by: Andri Yngvason <andri.yngvason@marel.com>
>> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
>> ---
>> drivers/net/can/usb/kvaser_usb.c | 114 +++++++++++++++++++--------------------
>> 1 file changed, 55 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
>> index 971c5f9..0386d3f 100644
>> --- a/drivers/net/can/usb/kvaser_usb.c
>> +++ b/drivers/net/can/usb/kvaser_usb.c
> Looks good.
Is this an Acked-by?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Patrick McHardy @ 2015-01-21 10:40 UTC (permalink / raw)
To: Thomas Graf; +Cc: Herbert Xu, davem, paulmck, ying.xue, netdev, netfilter-devel
In-Reply-To: <20150121103421.GR20315@casper.infradead.org>
On 21.01, Thomas Graf wrote:
> On 01/21/15 at 08:58pm, Herbert Xu wrote:
> > On Wed, Jan 21, 2015 at 09:49:28AM +0000, Thomas Graf wrote:
> > >
> > > An entry can move between different tables and thus chains need to be
> > > marked to identify what list a lookup ended up searching in. It's not
> > > the nulls marker itself that is needed, it's the bits in the last next
> > > pointer identifying the list that the nulls marker allows to be used
> > > which are essential.
> >
> > Can you describe in more detail how it's going to be used? I don't
> > see how I could use the bit if you need it to indicate the end of
> > the list.
>
> Another thought: Instead of storing that bit in the next pointer, we
> could require the user to store this bit, i.e. two new function
> pointers to rhashtable_params, set_walk_bit() and get_walk_bit(),
> which take the hashed object as argument and if a rhashtable user
> requires consistent dumps he can provide these functions to store
> the flag.
On the danger of repeating myself, every (converted) user requires
that we at least keep the existing semantics since it is exposed to
userspace. My opinion is that NLM_F_DUMP_INTR is fine if userspace
indicates support, but without that, we have to take care of that
in the kernel.
An automatic restart handles this well. Userspace always had to
expect duplicates.
^ permalink raw reply
* Re: [PATCH net] ipv4: try to cache dst_entries which would cause a redirect
From: Hannes Frederic Sowa @ 2015-01-21 10:39 UTC (permalink / raw)
To: Julian Anastasov; +Cc: netdev, Marcelo Leitner, Florian Westphal
In-Reply-To: <alpine.LFD.2.11.1501211041360.2329@ja.home.ssi.bg>
Hi Julian,
On Mi, 2015-01-21 at 10:56 +0200, Julian Anastasov wrote:
> On Tue, 20 Jan 2015, Hannes Frederic Sowa wrote:
>
> > Not caching dst_entries which cause redirects could be exploited by hosts
> > on the same subnet, causing a severe DoS attack. This effect aggravated
> > since commit f88649721268999 ("ipv4: fix dst race in sk_dst_get()").
> >
> > Lookups causing redirects will be allocated with DST_NOCACHE set which
> > will force dst_release to free them via RCU. Unfortunately waiting for
> > RCU grace period just takes too long, we can end up with >1M dst_entries
> > waiting to be released and the system will run OOM. rcuos threads cannot
> > catch up under high softirq load.
> >
> > Attaching the flag to emit a redirect later on to the specific skb allows
> > us to cache those dst_entries thus reducing the pressure on allocation
> > and deallocation.
> >
> > This issue was discovered by Marcelo Leitner.
>
> Change looks good to me but additional place
> should be changed too: inet_rtm_getroute() will call
> ip_route_input() and later rt_fill_info() will put
> rt_flags in rtm_flags. We have to set RTCF_DOREDIRECT
> just in rtm_flags depending on IPSKB_DOREDIRECT becuase
> iproute needs to print "redirect". You can test it with
> ip route get ... iif INDEV
Very good catch, thanks. Will post v2 soon.
Thanks,
Hannes
^ permalink raw reply
* RE: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: David Laight @ 2015-01-21 10:36 UTC (permalink / raw)
To: 'Thomas Graf', Herbert Xu
Cc: Patrick McHardy, davem@davemloft.net, paulmck@linux.vnet.ibm.com,
ying.xue@windriver.com, netdev@vger.kernel.org,
netfilter-devel@vger.kernel.org
In-Reply-To: <20150121093722.GM20315@casper.infradead.org>
From: Thomas Graf
> On 01/21/15 at 04:08pm, Herbert Xu wrote:
...
> > Essentially I need a bit to indicate an entry in the bucket chain
> > should be skipped, either because it has just been removed or that
> > it is a walker entry (see xfrm_state_walk).
> >
> > The way it'll work then is exactly the same as xfrm_state_walk,
> > except that the linked list is broken up into individual buckets.
> >
> > Of course we'll still need to postpone resizes (and rehashes which
> > is what my work is about) during a walk but I think that's a fair
> > price to pay.
>
> If I understand this correctly we also need to block out parallel
> walkers and we need to start taking bucket locks while walking to
> modify the walker mark bit in peace.
Running 'walker | grep foo | less' really shouldn't stop any activity,
including further requests by the same person to dump the same table in a
different window.
I would also expect the above request to be able to continue correctly
when restarted hours later (unless something unusual - like a resize -
happens).
Suppressing 'table shrink' because you think a walker might be active
if probably a good idea, and just requires a timestamp of the last
walk action.
Thought...
It is possible to break walking only on hash chain boundaries?
At least in the case where the response buffer is large enough
for a full 'chain' of entries.
Also what happens if there is a page fault on any userpage?
You don't want to be holding a mutex.
David
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Thomas Graf @ 2015-01-21 10:34 UTC (permalink / raw)
To: Herbert Xu
Cc: Patrick McHardy, davem, paulmck, ying.xue, netdev,
netfilter-devel
In-Reply-To: <20150121095837.GA25750@gondor.apana.org.au>
On 01/21/15 at 08:58pm, Herbert Xu wrote:
> On Wed, Jan 21, 2015 at 09:49:28AM +0000, Thomas Graf wrote:
> >
> > An entry can move between different tables and thus chains need to be
> > marked to identify what list a lookup ended up searching in. It's not
> > the nulls marker itself that is needed, it's the bits in the last next
> > pointer identifying the list that the nulls marker allows to be used
> > which are essential.
>
> Can you describe in more detail how it's going to be used? I don't
> see how I could use the bit if you need it to indicate the end of
> the list.
Another thought: Instead of storing that bit in the next pointer, we
could require the user to store this bit, i.e. two new function
pointers to rhashtable_params, set_walk_bit() and get_walk_bit(),
which take the hashed object as argument and if a rhashtable user
requires consistent dumps he can provide these functions to store
the flag.
^ permalink raw reply
* Re: [PATCH v5 2/5] can: kvaser_usb: Consolidate and unify state change handling
From: Andri Yngvason @ 2015-01-21 10:33 UTC (permalink / raw)
To: Ahmed S. Darwish, Olivier Sobrie, Oliver Hartkopp,
Wolfgang Grandegger, Marc Kleine-Budde
Cc: Linux-CAN, netdev, LKML
In-Reply-To: <20150120214537.GB16828@linux>
Quoting Ahmed S. Darwish (2015-01-20 21:45:37)
> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
>
> Replace most of the can interface's state and error counters
> handling with the new can-dev can_change_state() mechanism.
>
> Suggested-by: Andri Yngvason <andri.yngvason@marel.com>
> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> ---
> drivers/net/can/usb/kvaser_usb.c | 114 +++++++++++++++++++--------------------
> 1 file changed, 55 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> index 971c5f9..0386d3f 100644
> --- a/drivers/net/can/usb/kvaser_usb.c
> +++ b/drivers/net/can/usb/kvaser_usb.c
> @@ -620,40 +620,43 @@ static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
> }
>
> static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *priv,
> - const struct kvaser_usb_error_summary *es)
> + const struct kvaser_usb_error_summary *es,
> + struct can_frame *cf)
> {
> struct net_device_stats *stats;
> - enum can_state new_state;
> -
> - stats = &priv->netdev->stats;
> - new_state = priv->can.state;
> + enum can_state cur_state, new_state, tx_state, rx_state;
>
> netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status);
>
> - if (es->status & M16C_STATE_BUS_OFF) {
> - priv->can.can_stats.bus_off++;
> + stats = &priv->netdev->stats;
> + new_state = cur_state = priv->can.state;
> +
> + if (es->status & M16C_STATE_BUS_OFF)
> new_state = CAN_STATE_BUS_OFF;
> - } else if (es->status & M16C_STATE_BUS_PASSIVE) {
> - if (priv->can.state != CAN_STATE_ERROR_PASSIVE)
> - priv->can.can_stats.error_passive++;
> + else if (es->status & M16C_STATE_BUS_PASSIVE)
> new_state = CAN_STATE_ERROR_PASSIVE;
> - }
>
> if (es->status == M16C_STATE_BUS_ERROR) {
> - if ((priv->can.state < CAN_STATE_ERROR_WARNING) &&
> - ((es->txerr >= 96) || (es->rxerr >= 96))) {
> - priv->can.can_stats.error_warning++;
> + if ((cur_state < CAN_STATE_ERROR_WARNING) &&
> + ((es->txerr >= 96) || (es->rxerr >= 96)))
> new_state = CAN_STATE_ERROR_WARNING;
> - } else if (priv->can.state > CAN_STATE_ERROR_ACTIVE) {
> + else if (cur_state > CAN_STATE_ERROR_ACTIVE)
> new_state = CAN_STATE_ERROR_ACTIVE;
> - }
> }
>
> if (!es->status)
> new_state = CAN_STATE_ERROR_ACTIVE;
>
> + if (new_state != cur_state) {
> + tx_state = (es->txerr >= es->rxerr) ? new_state : 0;
> + rx_state = (es->txerr <= es->rxerr) ? new_state : 0;
> +
> + can_change_state(priv->netdev, cf, tx_state, rx_state);
> + new_state = priv->can.state;
> + }
> +
> if (priv->can.restart_ms &&
> - (priv->can.state >= CAN_STATE_BUS_OFF) &&
> + (cur_state >= CAN_STATE_BUS_OFF) &&
> (new_state < CAN_STATE_BUS_OFF)) {
> priv->can.can_stats.restarts++;
> }
> @@ -665,18 +668,17 @@ static void kvaser_usb_rx_error_update_can_state(struct kvaser_usb_net_priv *pri
>
> priv->bec.txerr = es->txerr;
> priv->bec.rxerr = es->rxerr;
> - priv->can.state = new_state;
> }
>
> static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> const struct kvaser_msg *msg)
> {
> - struct can_frame *cf;
> + struct can_frame *cf, tmp_cf = { .can_id = CAN_ERR_FLAG, .can_dlc = CAN_ERR_DLC };
> struct sk_buff *skb;
> struct net_device_stats *stats;
> struct kvaser_usb_net_priv *priv;
> struct kvaser_usb_error_summary es = { };
> - enum can_state old_state;
> + enum can_state old_state, new_state;
>
> switch (msg->id) {
> case CMD_CAN_ERROR_EVENT:
> @@ -721,60 +723,54 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> }
>
> /* Update all of the can interface's state and error counters before
> - * trying any skb allocation that can actually fail with -ENOMEM.
> + * trying any memory allocation that can actually fail with -ENOMEM.
> + *
> + * We send a temporary stack-allocated error can frame to
> + * can_change_state() for the very same reason.
> + *
> + * TODO: Split can_change_state() responsibility between updating the
> + * can interface's state and counters, and the setting up of can error
> + * frame ID and data to userspace. Remove stack allocation afterwards.
> */
> old_state = priv->can.state;
> - kvaser_usb_rx_error_update_can_state(priv, &es);
> + kvaser_usb_rx_error_update_can_state(priv, &es, &tmp_cf);
> + new_state = priv->can.state;
>
> skb = alloc_can_err_skb(priv->netdev, &cf);
> if (!skb) {
> stats->rx_dropped++;
> return;
> }
> + memcpy(cf, &tmp_cf, sizeof(*cf));
>
> - if (es.status & M16C_STATE_BUS_OFF) {
> - cf->can_id |= CAN_ERR_BUSOFF;
> -
> - if (!priv->can.restart_ms)
> - kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP);
> - netif_carrier_off(priv->netdev);
> - } else if (es.status & M16C_STATE_BUS_PASSIVE) {
> - if (old_state != CAN_STATE_ERROR_PASSIVE) {
> - cf->can_id |= CAN_ERR_CRTL;
> -
> - if (es.txerr || es.rxerr)
> - cf->data[1] = (es.txerr > es.rxerr)
> - ? CAN_ERR_CRTL_TX_PASSIVE
> - : CAN_ERR_CRTL_RX_PASSIVE;
> - else
> - cf->data[1] = CAN_ERR_CRTL_TX_PASSIVE |
> - CAN_ERR_CRTL_RX_PASSIVE;
> + if (new_state != old_state) {
> + if (es.status & M16C_STATE_BUS_OFF) {
> + if (!priv->can.restart_ms)
> + kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP);
> + netif_carrier_off(priv->netdev);
> + }
> +
> + if (es.status == M16C_STATE_BUS_ERROR) {
> + if ((old_state >= CAN_STATE_ERROR_WARNING) ||
> + (es.txerr < 96 && es.rxerr < 96)) {
> + if (old_state > CAN_STATE_ERROR_ACTIVE) {
> + cf->can_id |= CAN_ERR_PROT;
> + cf->data[2] = CAN_ERR_PROT_ACTIVE;
> + }
> + }
> }
> - }
>
> - if (es.status == M16C_STATE_BUS_ERROR) {
> - if ((old_state < CAN_STATE_ERROR_WARNING) &&
> - ((es.txerr >= 96) || (es.rxerr >= 96))) {
> - cf->can_id |= CAN_ERR_CRTL;
> - cf->data[1] = (es.txerr > es.rxerr)
> - ? CAN_ERR_CRTL_TX_WARNING
> - : CAN_ERR_CRTL_RX_WARNING;
> - } else if (old_state > CAN_STATE_ERROR_ACTIVE) {
> + if (!es.status) {
> cf->can_id |= CAN_ERR_PROT;
> cf->data[2] = CAN_ERR_PROT_ACTIVE;
> }
> - }
>
> - if (!es.status) {
> - cf->can_id |= CAN_ERR_PROT;
> - cf->data[2] = CAN_ERR_PROT_ACTIVE;
> - }
> -
> - if (priv->can.restart_ms &&
> - (old_state >= CAN_STATE_BUS_OFF) &&
> - (priv->can.state < CAN_STATE_BUS_OFF)) {
> - cf->can_id |= CAN_ERR_RESTARTED;
> - netif_carrier_on(priv->netdev);
> + if (priv->can.restart_ms &&
> + (old_state >= CAN_STATE_BUS_OFF) &&
> + (new_state < CAN_STATE_BUS_OFF)) {
> + cf->can_id |= CAN_ERR_RESTARTED;
> + netif_carrier_on(priv->netdev);
> + }
> }
>
> if (es.error_factor) {
> --
> 1.9.1
Looks good.
--
Andri
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Thomas Graf @ 2015-01-21 10:23 UTC (permalink / raw)
To: Herbert Xu
Cc: Patrick McHardy, davem, paulmck, ying.xue, netdev,
netfilter-devel
In-Reply-To: <20150121095837.GA25750@gondor.apana.org.au>
On 01/21/15 at 08:58pm, Herbert Xu wrote:
> On Wed, Jan 21, 2015 at 09:49:28AM +0000, Thomas Graf wrote:
> >
> > An entry can move between different tables and thus chains need to be
> > marked to identify what list a lookup ended up searching in. It's not
> > the nulls marker itself that is needed, it's the bits in the last next
> > pointer identifying the list that the nulls marker allows to be used
> > which are essential.
>
> Can you describe in more detail how it's going to be used? I don't
> see how I could use the bit if you need it to indicate the end of
> the list.
The usage will be identical to how __inet_lookup_listener() uses it.
If at the end of the lookup, we ended up in a different table than
we started, the lookup is restarted as an entry has moved to another
table while we were moving over it.
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Patrick McHardy @ 2015-01-21 10:00 UTC (permalink / raw)
To: Thomas Graf; +Cc: Herbert Xu, davem, paulmck, ying.xue, netdev, netfilter-devel
In-Reply-To: <20150121095634.GO20315@casper.infradead.org>
On 21.01, Thomas Graf wrote:
> On 01/21/15 at 08:14pm, Herbert Xu wrote:
> > On Wed, Jan 21, 2015 at 04:15:55PM +1100, Herbert Xu wrote:
> > > I failed to address the security aspect of this approach. Obviously
> > > this can only work if the only entites doing the walk are trusted.
> > > Which means that the vast majority (if not all) hash table users
> > > would be excluded, in particular, netlink.
> >
> > OK maybe we can get around this. So we will postpone the resize
> > or rehash when a walk is in place, however, when we hit a hard
> > limit, i.e., when insert would otherwise fail, then we do the
> > rehash regardless of any outstanding walks. The walks will just
> > behave erratically or we could force them to start from scratch
> > again.
>
> Exactly. I think we also need a timer to abort walks because if
> only a single walker is allowed, an attacker could start a walk and
> not complete it to block out everybody else.
Restarting them automatically sounds reasonable, duplicate entries have
always been possible. That makes me think, we could have userspace
indicate support for NLM_F_DUMP_INTR and otherwise always restart.
That would solve the problem very easily.
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Herbert Xu @ 2015-01-21 9:59 UTC (permalink / raw)
To: Thomas Graf
Cc: Patrick McHardy, davem, paulmck, ying.xue, netdev,
netfilter-devel
In-Reply-To: <20150121095634.GO20315@casper.infradead.org>
On Wed, Jan 21, 2015 at 09:56:34AM +0000, Thomas Graf wrote:
>
> Exactly. I think we also need a timer to abort walks because if
> only a single walker is allowed, an attacker could start a walk and
> not complete it to block out everybody else.
My scheme should support an arbitrary number of walks. See how
xfrm_state_walk is implemented.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Herbert Xu @ 2015-01-21 9:58 UTC (permalink / raw)
To: Thomas Graf
Cc: Patrick McHardy, davem, paulmck, ying.xue, netdev,
netfilter-devel
In-Reply-To: <20150121094928.GN20315@casper.infradead.org>
On Wed, Jan 21, 2015 at 09:49:28AM +0000, Thomas Graf wrote:
>
> An entry can move between different tables and thus chains need to be
> marked to identify what list a lookup ended up searching in. It's not
> the nulls marker itself that is needed, it's the bits in the last next
> pointer identifying the list that the nulls marker allows to be used
> which are essential.
Can you describe in more detail how it's going to be used? I don't
see how I could use the bit if you need it to indicate the end of
the list.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Thomas Graf @ 2015-01-21 9:56 UTC (permalink / raw)
To: Herbert Xu
Cc: Patrick McHardy, davem, paulmck, ying.xue, netdev,
netfilter-devel
In-Reply-To: <20150121091457.GA25289@gondor.apana.org.au>
On 01/21/15 at 08:14pm, Herbert Xu wrote:
> On Wed, Jan 21, 2015 at 04:15:55PM +1100, Herbert Xu wrote:
> > I failed to address the security aspect of this approach. Obviously
> > this can only work if the only entites doing the walk are trusted.
> > Which means that the vast majority (if not all) hash table users
> > would be excluded, in particular, netlink.
>
> OK maybe we can get around this. So we will postpone the resize
> or rehash when a walk is in place, however, when we hit a hard
> limit, i.e., when insert would otherwise fail, then we do the
> rehash regardless of any outstanding walks. The walks will just
> behave erratically or we could force them to start from scratch
> again.
Exactly. I think we also need a timer to abort walks because if
only a single walker is allowed, an attacker could start a walk and
not complete it to block out everybody else.
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Thomas Graf @ 2015-01-21 9:49 UTC (permalink / raw)
To: Herbert Xu
Cc: Patrick McHardy, davem, paulmck, ying.xue, netdev,
netfilter-devel
In-Reply-To: <20150121093836.GA25489@gondor.apana.org.au>
On 01/21/15 at 08:38pm, Herbert Xu wrote:
> On Wed, Jan 21, 2015 at 09:37:22AM +0000, Thomas Graf wrote:
> >
> > Without having seen your code, can we make it configurable on what
> > the bit is used for? Use of nulls marker is a strict requirement for
> > some targeted users of rhashtable.
>
> What do they need this for?
An entry can move between different tables and thus chains need to be
marked to identify what list a lookup ended up searching in. It's not
the nulls marker itself that is needed, it's the bits in the last next
pointer identifying the list that the nulls marker allows to be used
which are essential.
This is on my plate next. Most of the work in rhashtable was done in
preparation of translating the TCP established table over to
rhashtable.
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Herbert Xu @ 2015-01-21 9:38 UTC (permalink / raw)
To: Thomas Graf
Cc: Patrick McHardy, davem, paulmck, ying.xue, netdev,
netfilter-devel
In-Reply-To: <20150121093722.GM20315@casper.infradead.org>
On Wed, Jan 21, 2015 at 09:37:22AM +0000, Thomas Graf wrote:
>
> Without having seen your code, can we make it configurable on what
> the bit is used for? Use of nulls marker is a strict requirement for
> some targeted users of rhashtable.
What do they need this for?
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 3/3] netlink: Lock out table resizes while dumping Netlink sockets
From: Thomas Graf @ 2015-01-21 9:37 UTC (permalink / raw)
To: Herbert Xu
Cc: Patrick McHardy, davem, paulmck, ying.xue, netdev,
netfilter-devel
In-Reply-To: <20150121050819.GA23062@gondor.apana.org.au>
On 01/21/15 at 04:08pm, Herbert Xu wrote:
> On Tue, Jan 20, 2015 at 03:35:56PM +0000, Thomas Graf wrote:
> > On 01/20/15 at 03:21pm, Patrick McHardy wrote:
> > > I think its preferrable to make the need to handle NETLINK_F_DUMP_INTR
> > > as noticable as possible and not hide it. Silent failure is the worst
> > > kind of failure.
> >
> > I agree to that. The point here is to avoid unnecessary use of
> > NETLINK_F_DUMP_INTR if all entries fit into a single message buffer.
>
> OK I think I have a solution for you guys. But first you'll need to
> wait for me to undo the nulls stuff so I can steal that bit which
> is central to my solution.
Without having seen your code, can we make it configurable on what
the bit is used for? Use of nulls marker is a strict requirement for
some targeted users of rhashtable.
> Essentially I need a bit to indicate an entry in the bucket chain
> should be skipped, either because it has just been removed or that
> it is a walker entry (see xfrm_state_walk).
>
> The way it'll work then is exactly the same as xfrm_state_walk,
> except that the linked list is broken up into individual buckets.
>
> Of course we'll still need to postpone resizes (and rehashes which
> is what my work is about) during a walk but I think that's a fair
> price to pay.
If I understand this correctly we also need to block out parallel
walkers and we need to start taking bucket locks while walking to
modify the walker mark bit in peace.
> This also means handling insertion failures but I think that
> should be acceptable if we make it based on a configurable maximum
> chain length along with forced resize/rehash where possible.
>
> Note that this can be made optional, i.e., if the user can afford
> memory to do their own walking (e.g., xfrm_state), then none of
> this needs to happen and it'll just work as it does now. IOW if
> you don't use this special rhashtable walk function then you're
> not affected.
That sounds like the best option to me.
^ permalink raw reply
* Re: [PATCH 1/2] if_link: Add VF multicast promiscuous mode control
From: Bjørn Mork @ 2015-01-21 9:26 UTC (permalink / raw)
To: Hiroshi Shimamoto
Cc: e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org,
Choi, Sy Jong, linux-kernel@vger.kernel.org, Hayato Momma
In-Reply-To: <7F861DC0615E0C47A872E6F3C5FCDDBD05E07B7C@BPXM14GP.gisp.nec.co.jp>
Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com> writes:
>> Why can't the ixgbevf driver just automatically signal the ixgbe driver
>> to enable multicast promiscuous mode whenever the list grows past the
>> limit?
>
> I had submitted a patch to change ixgbe and ixgbevf driver for this issue.
> https://lkml.org/lkml/2014/11/27/269
>
> The previous patch introduces API between ixgbe and ixgbevf driver to
> enable multicast promiscuous mode, and ixgbevf enables it automatically
> if the number of addresses is over than 30.
>
> I got some comment and I would like to clarify the point, but there was no
> answer.
> That's the reason I submitted this patch.
Thanks. Yes, now I understand why you want to have a policy knob.
I still think the policy could select between "automatic"/"disallowed"
instead of "enabled"/"disabled", but that's a minor detail. Likewise is
the actual implemention of "automatic". I think you could do that
within the current VF-PF protocol by overloading the MC address "count".
But a more generic question for netdev is: Does this VF policy API
really scale?
How many different VF policy tunables can you imaging add up over a few
years and drivers. Currently each policy flag require its own ndo hook.
I probably don't have much to say here, but IMHO this scheme had already
failed when .ndo_set_vf_spoofchk was added..
Bjørn
------------------------------------------------------------------------------
New Year. New Location. New Benefits. New Data Center in Ashburn, VA.
GigeNET is offering a free month of service with a new server in Ashburn.
Choose from 2 high performing configs, both with 100TB of bandwidth.
Higher redundancy.Lower latency.Increased capacity.Completely compliant.
http://p.sf.net/sfu/gigenet
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox