* Re: [PATCH V2 5/7] sctp: use limited socket backlog
From: Vlad Yasevich @ 2010-03-03 14:10 UTC (permalink / raw)
To: Zhu Yi; +Cc: netdev, Sridhar Samudrala
In-Reply-To: <1267605389-7369-5-git-send-email-yi.zhu@intel.com>
Zhu Yi wrote:
> Make sctp adapt to the limited socket backlog change.
>
> Cc: Vlad Yasevich <vladislav.yasevich@hp.com>
> Cc: Sridhar Samudrala <sri@us.ibm.com>
> Signed-off-by: Zhu Yi <yi.zhu@intel.com>
> ---
> net/sctp/input.c | 12 ++++++++----
> 1 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index c0c973e..20e69c3 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -75,7 +75,7 @@ static struct sctp_association *__sctp_lookup_association(
> const union sctp_addr *peer,
> struct sctp_transport **pt);
>
> -static void sctp_add_backlog(struct sock *sk, struct sk_buff *skb);
> +static int sctp_add_backlog(struct sock *sk, struct sk_buff *skb);
>
>
> /* Calculate the SCTP checksum of an SCTP packet. */
> @@ -265,8 +265,12 @@ int sctp_rcv(struct sk_buff *skb)
> }
>
> if (sock_owned_by_user(sk)) {
> + if (sctp_add_backlog(sk, skb)) {
> + sctp_bh_unlock_sock(sk);
> + sctp_chunk_free(chunk);
> + goto discard_release;
> + }
I think this will result in a double-free of the skb, because sctp_chunk_free
attempts to free the skb that's been assigned to the chunk. You can set the skb
to NULL to get around that.
> SCTP_INC_STATS_BH(SCTP_MIB_IN_PKT_BACKLOG);
> - sctp_add_backlog(sk, skb);
> } else {
> SCTP_INC_STATS_BH(SCTP_MIB_IN_PKT_SOFTIRQ);
> sctp_inq_push(&chunk->rcvr->inqueue, chunk);
> @@ -362,7 +366,7 @@ done:
> return 0;
> }
>
> -static void sctp_add_backlog(struct sock *sk, struct sk_buff *skb)
> +static int sctp_add_backlog(struct sock *sk, struct sk_buff *skb)
> {
> struct sctp_chunk *chunk = SCTP_INPUT_CB(skb)->chunk;
> struct sctp_ep_common *rcvr = chunk->rcvr;
> @@ -377,7 +381,7 @@ static void sctp_add_backlog(struct sock *sk, struct sk_buff *skb)
> else
> BUG();
>
> - sk_add_backlog(sk, skb);
> + return sk_add_backlog_limited(sk, skb);
> }
You also leak the ref counts here since now it's possible to not add a packet to
the backlog queue. That means you'll take refs, but never drop them because
the receive routing will never run.
-vlad
>
> /* Handle icmp frag needed error. */
^ permalink raw reply
* RE: [PATCH V2 2/7] tcp: use limited socket backlog
From: Zhu, Yi @ 2010-03-03 14:12 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev@vger.kernel.org, David S. Miller, Alexey Kuznetsov,
Pekka Savola (ipv6), Patrick McHardy
In-Reply-To: <1267610843.2997.7.camel@edumazet-laptop>
Eric Dumazet [mailto:eric.dumazet@gmail.com] wrote:
> Le mercredi 03 mars 2010 à 17:06 +0800, Zhu Yi a écrit :
>> I simply follow how the code is originally written. As you can see,
>> tcp_v4_do_rcv() doesn't always do so. And in the backlog queuing place,
>> we don't even bother to check.
> You add a new point where a packet can be dropped, this should be
> accounted for, so that admins can have a clue whats going on.
> Previously, packet was always queued, and dropped later (and accounted)
In case of the skb doesn't have a MD5 option while we are expecting one, or we
failed to find the sk for the skb connection request, etc, the skb is dropped silently in
tcp_v4_do_rcv(). No?
> Not everybody runs drop monitor :)
Thanks,
-yi
^ permalink raw reply
* RE: [PATCH V2 5/7] sctp: use limited socket backlog
From: Zhu, Yi @ 2010-03-03 14:19 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: netdev@vger.kernel.org, Sridhar Samudrala
In-Reply-To: <4B8E6DD0.1020603@hp.com>
Vlad Yasevich <vladislav.yasevich@hp.com> wrote:
> I think this will result in a double-free of the skb, because sctp_chunk_free
> attempts to free the skb that's been assigned to the chunk. You can set the skb
> to NULL to get around that.
Ah, I missed that. Thanks!
<...>
> You also leak the ref counts here since now it's possible to not add a packet to
> the backlog queue. That means you'll take refs, but never drop them because
> the receive routing will never run.
Good catch. I'll fix it.
BTW, does the current backlog limit (sysctl_rmem_default[1] << 1) enough for sctp?
I noticed the sysctl_sctp_rmem[1] is set to 373500 in my box.
Thanks,
-yi
^ permalink raw reply
* Re: [PATCH 2/2] ipv4: flush ARP entries on device change
From: Timo Teräs @ 2010-03-03 14:20 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netdev
In-Reply-To: <4B8E6CC8.3080701@trash.net>
Patrick McHardy wrote:
> Timo Teras wrote:
>> If device flag IFF_NOARP is changed, we should flush the ARP cache as all
>> entries need to get refreshed.
>>
>> Signed-off-by: Timo Teras <timo.teras@iki.fi>
>> ---
>> net/ipv4/arp.c | 3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
>> index c4dd135..036da92 100644
>> --- a/net/ipv4/arp.c
>> +++ b/net/ipv4/arp.c
>> @@ -1245,6 +1245,9 @@ static int arp_netdev_event(struct notifier_block *this, unsigned long event, vo
>> neigh_changeaddr(&arp_tbl, dev);
>> rt_cache_flush(dev_net(dev), 0);
>> break;
>> + case NETDEV_CHANGE:
>> + neigh_changeaddr(&arp_tbl, dev);
>> + break;
>
> It would be nice if we could restrict this to IFF_NOARP changes.
Yes. But I did not see any easy way to figure out which flags have changed.
Should we just keep a copy of the previous IFF_NOARP bit somewhere (where?).
Or did I miss something obvious?
- Timo
^ permalink raw reply
* Re: e1000e: reset of tx_queue_len
From: Franco Fichtner @ 2010-03-03 14:22 UTC (permalink / raw)
To: Brandeburg, Jesse; +Cc: Franco Fichtner, Netdev, Allan, Bruce W
In-Reply-To: <alpine.WNT.2.00.1003021020160.2888@jbrandeb-desk1.amr.corp.intel.com>
Hi Jesse,
> Its possible we should just remove this code from the in-kernel
> driver, as I don't think many other drivers mess with the tx_queue_len
> either. We did introduce the code to fix some users complaints about the
> amount of work that can get queued up in the hardware with tx_queue_len =
> 1000 when at 10/100 speeds.
>
I've tested the changes you suggested and now tx_queue_len
is no longer reset. I have also prepared a patch for net-next
for you to sign or ack (or nack) because I'd like to see this go,
also.
Thanks again,
Franco
^ permalink raw reply
* [net-next-2.6 PATCH] e1000e: do not modify tx queue len on link speed change
From: franco @ 2010-03-03 14:31 UTC (permalink / raw)
To: Brandeburg, Jesse; +Cc: Netdev, Allan, Bruce W
e1000e would try to set appropriate values of tx_queue_len for
negotiated
link speeds overwriting the already set configuration of tx_queue_len in
the process. This removes all lines touching tx_queue_len from the
driver.
Signed-off-by: Franco Fichtner <franco@lastsummer.de>
---
drivers/net/e1000e/netdev.c | 11 +----------
1 files changed, 1 insertions(+), 10 deletions(-)
diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 88d54d3..e1cceb6 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -2289,8 +2289,6 @@ static void e1000_configure_tx(struct
e1000_adapter *adapter)
ew32(TCTL, tctl);
e1000e_config_collision_dist(hw);
-
- adapter->tx_queue_len = adapter->netdev->tx_queue_len;
}
/**
@@ -2877,7 +2875,6 @@ void e1000e_down(struct e1000_adapter *adapter)
del_timer_sync(&adapter->watchdog_timer);
del_timer_sync(&adapter->phy_info_timer);
- netdev->tx_queue_len = adapter->tx_queue_len;
netif_carrier_off(netdev);
adapter->link_speed = 0;
adapter->link_duplex = 0;
@@ -3588,21 +3585,15 @@ static void e1000_watchdog_task(struct
work_struct *work)
"link gets many collisions.\n");
}
- /*
- * tweak tx_queue_len according to speed/duplex
- * and adjust the timeout factor
- */
- netdev->tx_queue_len = adapter->tx_queue_len;
+ /* adjust timeout factor according to speed/duplex */
adapter->tx_timeout_factor = 1;
switch (adapter->link_speed) {
case SPEED_10:
txb2b = 0;
- netdev->tx_queue_len = 10;
adapter->tx_timeout_factor = 16;
break;
case SPEED_100:
txb2b = 0;
- netdev->tx_queue_len = 100;
adapter->tx_timeout_factor = 10;
break;
}
--
^ permalink raw reply related
* RE: [PATCH 8/8] x25: use limited socket backlog
From: Eric Dumazet @ 2010-03-03 14:33 UTC (permalink / raw)
To: Zhu, Yi; +Cc: andrew hendry, netdev@vger.kernel.org
In-Reply-To: <DA586906BA1FFC4384FCFD6429ECE860A45A6992@shzsmsx502.ccr.corp.intel.com>
Le mercredi 03 mars 2010 à 22:00 +0800, Zhu, Yi a écrit :
> andrew hendry <andrew.hendry@gmail.com> wrote:
>
> > Will wait for the next spin and in the meantime think if there is way
> > to test it. x25 with no loopback and being so slow probably cant generate the same
> > as your UDP case.
>
> I didn't find a way to drop the packet correctly. So I didn't change any behavior in
> this patch. Nor did I do in the second spin. It will be fine if you also think x25 doesn't
> need to limit its backlog size.
So are we sure we cant flood X25 backlog, using X25 over IP ?
You discovered a _fatal_ flaw in backlog processing, we should close all
holes, not only UDP case. You can be sure many bad guys will inspect all
possibilities to bring down Linux hosts.
If you feel uncomfortable with a small limit, just stick a big one, like
256 packets, and you are 100% sure you wont break a protocol. If this
limit happens to be too small, we can change it later.
(No need to count bytes, since truesize includes kernel overhead, and
this overhead depends on 32/64 wide of host and kernel versions)
^ permalink raw reply
* RE: [PATCH V2 2/7] tcp: use limited socket backlog
From: Eric Dumazet @ 2010-03-03 14:31 UTC (permalink / raw)
To: Zhu, Yi
Cc: netdev@vger.kernel.org, David S. Miller, Alexey Kuznetsov,
Pekka Savola (ipv6), Patrick McHardy
In-Reply-To: <DA586906BA1FFC4384FCFD6429ECE860A45A6999@shzsmsx502.ccr.corp.intel.com>
Le mercredi 03 mars 2010 à 22:12 +0800, Zhu, Yi a écrit :
> Eric Dumazet [mailto:eric.dumazet@gmail.com] wrote:
> > Le mercredi 03 mars 2010 à 17:06 +0800, Zhu Yi a écrit :
>
> >> I simply follow how the code is originally written. As you can see,
> >> tcp_v4_do_rcv() doesn't always do so. And in the backlog queuing place,
> >> we don't even bother to check.
>
> > You add a new point where a packet can be dropped, this should be
> > accounted for, so that admins can have a clue whats going on.
>
> > Previously, packet was always queued, and dropped later (and accounted)
>
> In case of the skb doesn't have a MD5 option while we are expecting one, or we
> failed to find the sk for the skb connection request, etc, the skb is dropped silently in
> tcp_v4_do_rcv(). No?
Then its a separate bug. MD5 support added so many bugs its not even
funny.
Existing bugs are not an excuse for adding new ones, we try the reverse.
No ?
^ permalink raw reply
* Re: [PATCH 2/2] ipv4: flush ARP entries on device change
From: Patrick McHardy @ 2010-03-03 14:33 UTC (permalink / raw)
To: Timo Teräs; +Cc: netdev
In-Reply-To: <4B8E701D.7070300@iki.fi>
Timo Teräs wrote:
> Patrick McHardy wrote:
>> Timo Teras wrote:
>>> If device flag IFF_NOARP is changed, we should flush the ARP cache as
>>> all
>>> entries need to get refreshed.
>>>
>>> Signed-off-by: Timo Teras <timo.teras@iki.fi>
>>> ---
>>> net/ipv4/arp.c | 3 +++
>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
>>> index c4dd135..036da92 100644
>>> --- a/net/ipv4/arp.c
>>> +++ b/net/ipv4/arp.c
>>> @@ -1245,6 +1245,9 @@ static int arp_netdev_event(struct
>>> notifier_block *this, unsigned long event, vo
>>> neigh_changeaddr(&arp_tbl, dev);
>>> rt_cache_flush(dev_net(dev), 0);
>>> break;
>>> + case NETDEV_CHANGE:
>>> + neigh_changeaddr(&arp_tbl, dev);
>>> + break;
>>
>> It would be nice if we could restrict this to IFF_NOARP changes.
>
> Yes. But I did not see any easy way to figure out which flags have changed.
>
> Should we just keep a copy of the previous IFF_NOARP bit somewhere
> (where?).
> Or did I miss something obvious?
We shouldn't have any arp entries for devices with IFF_NOARP set,
so perhaps we can flush only in that case. The transition IFF_NOARP
-> ~IFF_NOARP shouldn't need flushing.
^ permalink raw reply
* Re: [PATCH V2 5/7] sctp: use limited socket backlog
From: Vlad Yasevich @ 2010-03-03 14:36 UTC (permalink / raw)
To: Zhu, Yi; +Cc: netdev@vger.kernel.org, Sridhar Samudrala
In-Reply-To: <DA586906BA1FFC4384FCFD6429ECE860A45A699B@shzsmsx502.ccr.corp.intel.com>
Zhu, Yi wrote:
> Vlad Yasevich <vladislav.yasevich@hp.com> wrote:
>
>> I think this will result in a double-free of the skb, because sctp_chunk_free
>> attempts to free the skb that's been assigned to the chunk. You can set the skb
>> to NULL to get around that.
>
> Ah, I missed that. Thanks!
>
> <...>
>
>> You also leak the ref counts here since now it's possible to not add a packet to
>> the backlog queue. That means you'll take refs, but never drop them because
>> the receive routing will never run.
>
> Good catch. I'll fix it.
>
> BTW, does the current backlog limit (sysctl_rmem_default[1] << 1) enough for sctp?
> I noticed the sysctl_sctp_rmem[1] is set to 373500 in my box.
>
sctp uses the same algorithm as TCP to figure out the memory values.
I guess the issue with using the smaller value that it would be possible to
queue more the socket receive buffer then to the backlog. Thus backlog would
start dropping packets even though receive buffer would still accept them.
-vlad
> Thanks,
> -yi
>
^ permalink raw reply
* Re: [PATCH 2/2] ipv4: flush ARP entries on device change
From: Timo Teräs @ 2010-03-03 14:39 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netdev
In-Reply-To: <4B8E7357.5050203@trash.net>
Patrick McHardy wrote:
> Timo Teräs wrote:
>> Patrick McHardy wrote:
>>> Timo Teras wrote:
>>> It would be nice if we could restrict this to IFF_NOARP changes.
>> Yes. But I did not see any easy way to figure out which flags have changed.
>>
>> Should we just keep a copy of the previous IFF_NOARP bit somewhere
>> (where?).
>> Or did I miss something obvious?
>
> We shouldn't have any arp entries for devices with IFF_NOARP set,
> so perhaps we can flush only in that case. The transition IFF_NOARP
> -> ~IFF_NOARP shouldn't need flushing.
IFF_NOARP devices do have neighbor entries with the nud NOARP.
Exactly those entries I want to flush when IFF_NOARP flag is
removed.
You can see those entries with "ip neigh show nud all". You have
them e.g. for loopback stuff and broad-/multicast stuff in general.
With IFF_NOARP you get them on all unicast addresses used.
- Timo
^ permalink raw reply
* Re: [PATCH 2/2] ipv4: flush ARP entries on device change
From: Patrick McHardy @ 2010-03-03 14:44 UTC (permalink / raw)
To: Timo Teräs; +Cc: netdev
In-Reply-To: <4B8E74A8.5060600@iki.fi>
Timo Teräs wrote:
> Patrick McHardy wrote:
>> Timo Teräs wrote:
>>> Patrick McHardy wrote:
>>>> Timo Teras wrote:
>>>> It would be nice if we could restrict this to IFF_NOARP changes.
>>> Yes. But I did not see any easy way to figure out which flags have
>>> changed.
>>>
>>> Should we just keep a copy of the previous IFF_NOARP bit somewhere
>>> (where?).
>>> Or did I miss something obvious?
>>
>> We shouldn't have any arp entries for devices with IFF_NOARP set,
>> so perhaps we can flush only in that case. The transition IFF_NOARP
>> -> ~IFF_NOARP shouldn't need flushing.
>
> IFF_NOARP devices do have neighbor entries with the nud NOARP.
> Exactly those entries I want to flush when IFF_NOARP flag is
> removed.
>
> You can see those entries with "ip neigh show nud all". You have
> them e.g. for loopback stuff and broad-/multicast stuff in general.
> With IFF_NOARP you get them on all unicast addresses used.
I see. I don't have a better suggestion, except perhaps
to store the bit in dev->priv_flags.
^ permalink raw reply
* Re: [rfc/rft][patch] should use scheduler sync hint in tcp_prequeue()?
From: Mike Galbraith @ 2010-03-03 14:46 UTC (permalink / raw)
To: David Miller; +Cc: eric.dumazet, netdev, linux-kernel, mingo
In-Reply-To: <20100303.060529.195713861.davem@davemloft.net>
On Wed, 2010-03-03 at 06:05 -0800, David Miller wrote:
> It definitely looks like an oversight to me and we should add
> be using wake_up_interruptible_sync_poll() in tcp_prequeue()
> as above.
>
> Ingo?
Implied ack from network maintainer implies ship it to me.
net: add scheduler sync hint to tcp_prequeue().
Decreases the odds wakee will suffer from frequent cache misses.
Signed-off-by: Mike Galbraith <efault@gmx.de>
Cc: David Miller <davem@davemloft.net>
LKML-Reference: <new-submission>
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 34f5cc2..ba3fc64 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -939,7 +939,7 @@ static inline int tcp_prequeue(struct sock *sk, struct sk_buff *skb)
tp->ucopy.memory = 0;
} else if (skb_queue_len(&tp->ucopy.prequeue) == 1) {
- wake_up_interruptible_poll(sk->sk_sleep,
+ wake_up_interruptible_sync_poll(sk->sk_sleep,
POLLIN | POLLRDNORM | POLLRDBAND);
if (!inet_csk_ack_scheduled(sk))
inet_csk_reset_xmit_timer(sk, ICSK_TIME_DACK,
^ permalink raw reply related
* Re: [patch] cassini: fix off by one
From: Jaswinder Singh Rajput @ 2010-03-03 15:10 UTC (permalink / raw)
To: David Miller
Cc: error27, netdev, adobriyan, akpm, yanghy, kaber, linux-kernel,
kernel-janitors
In-Reply-To: <20100303.010746.134683403.davem@davemloft.net>
Hello Dave and Dan,
On Wed, Mar 3, 2010 at 2:37 PM, David Miller <davem@davemloft.net> wrote:
> From: Dan Carpenter <error27@gmail.com>
> Date: Wed, 3 Mar 2010 11:22:41 +0300
>
>> There are only 6 link_modes.
>>
>> Signed-off-by: Dan Carpenter <error27@gmail.com>
>
> Applied.
I am curious should we add these kind of email address in git log.
error27 seems error to me ;-)
Can we make Signed-off address little bit more sensible.
Thanks,
--
Jaswinder
^ permalink raw reply
* Re: [patch] cassini: fix off by one
From: David Miller @ 2010-03-03 15:17 UTC (permalink / raw)
To: jaswinderlinux
Cc: error27, netdev, adobriyan, akpm, yanghy, kaber, linux-kernel,
kernel-janitors
In-Reply-To: <3f9a31f41003030710p4209fa1dq37d14a67b12657d@mail.gmail.com>
From: Jaswinder Singh Rajput <jaswinderlinux@gmail.com>
Date: Wed, 3 Mar 2010 20:40:18 +0530
> Hello Dave and Dan,
>
> On Wed, Mar 3, 2010 at 2:37 PM, David Miller <davem@davemloft.net> wrote:
>> From: Dan Carpenter <error27@gmail.com>
>> Date: Wed, 3 Mar 2010 11:22:41 +0300
>>
>>> There are only 6 link_modes.
>>>
>>> Signed-off-by: Dan Carpenter <error27@gmail.com>
>>
>> Applied.
>
> I am curious should we add these kind of email address in git log.
> error27 seems error to me ;-)
>
> Can we make Signed-off address little bit more sensible.
Some people have silly email addresses, that is their choice.
I can't see any reason to apply censorship to an email address
just because the character sequences in it have some kind of
meaning.
The email address I was using up until 6 or 7 years ago used a host
name that meant "shit" in the Polish languange.
Maybe I should have been censored too?
^ permalink raw reply
* Re: Current wireless-testing breaks libpcap: mr_alen should be set
From: Frank W. Miller @ 2010-03-03 15:31 UTC (permalink / raw)
To: tcpdump-workers, linux-wireless, netdev; +Cc: 'Jiri Pirko'
In-Reply-To: <1267578048.14049.11.camel@mj>
Would this be preventing pcap_inject() from working say in kernel 2.6.31
(stock FC12 kernel)?
Thanks,
FM
> -----Original Message-----
> From: tcpdump-workers-owner@lists.tcpdump.org [mailto:tcpdump-workers-
> owner@lists.tcpdump.org] On Behalf Of Pavel Roskin
> Sent: Tuesday, March 02, 2010 6:01 PM
> To: linux-wireless@vger.kernel.org; netdev@vger.kernel.org; tcpdump-
> workers@lists.tcpdump.org
> Cc: Jiri Pirko
> Subject: [tcpdump-workers] Current wireless-testing breaks libpcap:
> mr_alen should be set
>
> Hello!
>
> The current wireless-testing appears to have some non-wireless bits from
> the upcoming Linux 2.6.34. As a result, libpcap and all capture
> programs that use it are broken.
>
> This patch to libpcap helps:
>
> --- a/pcap-linux.c
> +++ b/pcap-linux.c
> @@ -1563,6 +1563,7 @@ live_open_new(pcap_t *handle, const char
> memset(&mr, 0, sizeof(mr));
> mr.mr_ifindex = handle->md.ifindex;
> mr.mr_type = PACKET_MR_PROMISC;
> + mr.mr_alen = 6;
> if (setsockopt(sock_fd, SOL_PACKET,
> PACKET_ADD_MEMBERSHIP, &mr, sizeof(mr)) ==
-1)
> {
>
> libpcap git doesn't have the fix yet.
>
> The breakage must be coming from the commit 914c8ad2 by Jiri Pirko to
> net/packet/af_packet.c
>
> I think it's very unhelpful to introduce patches that break significant
> userspace functionality without giving the affected programs an advance
> warning.
>
> Also, pulling bleeding edge stuff into wireless-testing before rc1
> appears to be either a mistake or a bad decision.
>
> Sorry for cross-post, but it's an urgent issue. Repliers are encouraged
> to trim the recipient list as necessary.
>
> --
> Regards,
> Pavel Roskin
> -
> This is the tcpdump-workers list.
> Visit https://cod.sandelman.ca/ to unsubscribe.
^ permalink raw reply
* Re: [RFC][PATCH] ns: Syscalls for better namespace sharing control.
From: Serge E. Hallyn @ 2010-03-03 15:38 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Sukadev Bhattiprolu, Pavel Emelyanov, Linux Netdev List,
containers, Netfilter Development Mailinglist, Ben Greear
In-Reply-To: <m1ocj6qljj.fsf@fess.ebiederm.org>
Quoting Eric W. Biederman (ebiederm@xmission.com):
> Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> writes:
>
> > Eric W. Biederman [ebiederm@xmission.com] wrote:
> > |
> > | I think replacing a struct pid for another struct pid allocated in
> > | descendant pid_namespace (but has all of the same struct upid values
> > | as the first struct pid) is a disastrous idea. It destroys the
> >
> > True. Sorry, I did not mean we would need a new 'struct pid' for an
> > existing process. I think we talked earlier of finding a way of attaching
> > additional pid numbers to the same struct pid.
>
> I just played with this and if you make the semantics of unshare(CLONE_NEWPID)
> to be that you become the idle task aka pid 0, and not the init task pid 1 the
> implementation is trivial.
Heh, and then (browsing through your copy_process() patch hunks) the next
forked task becomes the child reaper for the new pidns? <shrug> why not
I guess.
Now if that child reaper then gets killed, will the idle task get killed too?
And if not, then idle task can just re-populating the new pidns with new
idle tasks...
If this brought us a step closer to entering an existing pidns that would
be one thing, but is there actually any advantage to being able to
unshare a new pidns? Oh, I guess there is - PAM can then use it at
login, which might be neat.
-serge
^ permalink raw reply
* Re: [tcpdump-workers] Current wireless-testing breaks libpcap: mr_alen should be set
From: Jiri Pirko @ 2010-03-03 15:54 UTC (permalink / raw)
To: Frank W. Miller; +Cc: tcpdump-workers, linux-wireless, netdev
In-Reply-To: <A5B3F074CA554D3E95CFD349116E4D5B@FrankWMillerPC>
Wed, Mar 03, 2010 at 04:31:07PM CET, frankwmiller@frankwmiller.net wrote:
>
>Would this be preventing pcap_inject() from working say in kernel 2.6.31
>(stock FC12 kernel)?
Nope. The patch went in just recently.
>
>Thanks,
>FM
>
>
>> -----Original Message-----
>> From: tcpdump-workers-owner@lists.tcpdump.org [mailto:tcpdump-workers-
>> owner@lists.tcpdump.org] On Behalf Of Pavel Roskin
>> Sent: Tuesday, March 02, 2010 6:01 PM
>> To: linux-wireless@vger.kernel.org; netdev@vger.kernel.org; tcpdump-
>> workers@lists.tcpdump.org
>> Cc: Jiri Pirko
>> Subject: [tcpdump-workers] Current wireless-testing breaks libpcap:
>> mr_alen should be set
>>
>> Hello!
>>
>> The current wireless-testing appears to have some non-wireless bits from
>> the upcoming Linux 2.6.34. As a result, libpcap and all capture
>> programs that use it are broken.
>>
>> This patch to libpcap helps:
>>
>> --- a/pcap-linux.c
>> +++ b/pcap-linux.c
>> @@ -1563,6 +1563,7 @@ live_open_new(pcap_t *handle, const char
>> memset(&mr, 0, sizeof(mr));
>> mr.mr_ifindex = handle->md.ifindex;
>> mr.mr_type = PACKET_MR_PROMISC;
>> + mr.mr_alen = 6;
>> if (setsockopt(sock_fd, SOL_PACKET,
>> PACKET_ADD_MEMBERSHIP, &mr, sizeof(mr)) ==
>-1)
>> {
>>
>> libpcap git doesn't have the fix yet.
>>
>> The breakage must be coming from the commit 914c8ad2 by Jiri Pirko to
>> net/packet/af_packet.c
>>
>> I think it's very unhelpful to introduce patches that break significant
>> userspace functionality without giving the affected programs an advance
>> warning.
>>
>> Also, pulling bleeding edge stuff into wireless-testing before rc1
>> appears to be either a mistake or a bad decision.
>>
>> Sorry for cross-post, but it's an urgent issue. Repliers are encouraged
>> to trim the recipient list as necessary.
>>
>> --
>> Regards,
>> Pavel Roskin
>> -
>> This is the tcpdump-workers list.
>> Visit https://cod.sandelman.ca/ to unsubscribe.
>
^ permalink raw reply
* Re: [net-next-2.6 PATCH] wireless: convert to use netdev_for_each_mc_addr
From: Jussi Kivilinna @ 2010-03-03 16:42 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem
In-Reply-To: <20100303124249.GD2961@psychotron.lab.eng.brq.redhat.com>
Hello!
Quoting "Jiri Pirko" <jpirko@redhat.com>:
> + } else if (mc_count) {
> + int size = min(priv->multicast_size, mc_count);
> + int i = 0;
> +
> + mc_addrs = kmalloc(size * ETH_ALEN, GFP_ATOMIC);
...
> + if (filter != basefilter)
> + goto set_filter;
> +
> + if (mc_count) {
> + ret = rndis_set_oid(usbdev, OID_802_3_MULTICAST_LIST, mc_addrs,
> + mc_count * ETH_ALEN);
> + kfree(mc_addrs);
mc_addrs was alloced by with 'size * ETH_ALEN', which might be less
than mc_count * ETH_ALEN.
Otherwise ok, and rndis_wlan works.
-Jussi
^ permalink raw reply
* Re: [PATCH]: tipc: Fix oops on send prior to entering networked mode (v2)
From: Stephens, Allan @ 2010-03-03 16:51 UTC (permalink / raw)
To: Neil Horman, netdev; +Cc: jon.maloy, tipc-discussion, davem
In-Reply-To: <20100302183312.GB22294@hmsreliant.think-freely.org>
Hi Neil:
Your patch looks pretty good to me, and is definitely a much cleaner
solution than what was discussed previously.
The only improvements I can suggest are the following:
- It looks like the check to see if tipc_bearers is NULL at the start of
tipc_bearer_stop() is unnecessary, since it's now a static array.
- The check to see if media_list is NULL at the start of
tipc_register_media() needs to be replaced with a check to ensure
tipc_mode is TIPC_NET_MODE. (Unlike the case with the tipc_bearers
check, we still need some sort of test in place to cause the routine to
fail if someone uses TIPC's native API to try registering an add-on
media type before TIPC is ready to handle it.)
Other than these minor points I can't see anything else that would cause
problems.
Regards,
Al
> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Tuesday, March 02, 2010 1:33 PM
> To: netdev@vger.kernel.org
> Cc: jon.maloy@ericsson.com; Stephens, Allan;
> tipc-discussion@lists.sourceforge.net; davem@davemloft.net
> Subject: Re: [PATCH]: tipc: Fix oops on send prior to
> entering networked mode (v2)
>
> Ok, after some debate, heres version 2 of this patch. Its a
> complete rewrite.
> I started with the patches we'd proposed previously, then
> realized theres a much easier and elegant way to handle this,
> which I've implemented below.
>
> Fix TIPC to disallow sending to remote addresses prior to
> entering NET_MODE
>
> user programs can oops the kernel by sending datagrams via
> AF_TIPC prior to entering networked mode. The following
> backtrace has been observed:
>
> ID: 13459 TASK: ffff810014640040 CPU: 0 COMMAND: "tipc-client"
> [exception RIP: tipc_node_select_next_hop+90]
> RIP: ffffffff8869d3c3 RSP: ffff81002d9a5ab8 RFLAGS: 00010202
> RAX: 0000000000000001 RBX: 0000000000000001 RCX: 0000000000000001
> RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000001001001
> RBP: 0000000001001001 R8: 0074736575716552 R9: 0000000000000000
> R10: ffff81003fbd0680 R11: 00000000000000c8 R12: 0000000000000008
> R13: 0000000000000001 R14: 0000000000000001 R15: ffff810015c6ca00
> ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
> #4 [ffff81002d9a5ab0] tipc_node_select_next_hop at ffffffff8869d3b1
> #5 [ffff81002d9a5ae0] tipc_link_send_sections_fast at ffffffff88698558
> #6 [ffff81002d9a5be0] tipc_forward2port at ffffffff8869eb1d
> #7 [ffff81002d9a5c10] tipc_send2port at ffffffff8869eb79
> #8 [ffff81002d9a5c30] send_msg at ffffffff886a1d0b
> #9 [ffff81002d9a5cb0] sock_sendmsg at ffffffff80055261
> RIP: 0000003cbd8d49a3 RSP: 00007fffc84e0be8 RFLAGS: 00010206
> RAX: 000000000000002c RBX: ffffffff8005d116 RCX: 0000000000000000
> RDX: 0000000000000008 RSI: 00007fffc84e0c00 RDI: 0000000000000003
> RBP: 0000000000000000 R8: 00007fffc84e0c10 R9: 0000000000000010
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> R13: 00007fffc84e0d10 R14: 0000000000000000 R15: 00007fffc84e0c30
> ORIG_RAX: 000000000000002c CS: 0033 SS: 002b
>
> What happens is that, when the tipc module in inserted it
> enters a standalone node mode in which communication to its
> own address is allowed <0.0.0> but not to other addresses,
> since the appropriate data structures have not been allocated
> yet (specifically the tipc_net pointer). There is nothing
> stopping a client from trying to send such a message however,
> and if that happens, we attempt to dereference tipc_net.zones
> while the pointer is still NULL, and explode. The fix is
> pretty straightforward. Since these oopses all arise from
> the dereference of global pointers prior to their assignment
> to allocated values, and since these allocations are small
> (about 2k total), lets convert these pointers to static
> arrays of the appropriate size. All the accesses to these
> bits consider 0/NULL to be a non match when searching, so all
> the lookups still work properly, and there is no longer a
> chance of a bad dererence anywhere. As a bonus, this lets us
> eliminate the setup/teardown routines for those pointers, and
> elimnates the need to preform any locking around them to
> prevent access while their being allocated/freed.
>
> I've updated the tipc_net structure to behave this way to fix
> the exact reported problem, and also fixed up the
> tipc_bearers and media_list arrays to fix an obvious simmilar
> problem that arises from issuing tipc-config commands to
> manipulate bearers/links prior to entering networked mode
>
> I've tested this for a few hours by running the sanity tests
> and stress test with the tipcutils suite, and nothing has
> fallen over. There have been a few lockdep warnings, but
> those were there before, and can be addressed later, as they
> didn't actually result in any deadlock.
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Allan Stephens <allan.stephens@windriver.com>
> CC: David S. Miller <davem@davemloft.net>
> CC: tipc-discussion@lists.sourceforge.net
>
>
> bearer.c | 28 ++--------------------------
> bearer.h | 2 +-
> net.c | 25 ++++---------------------
> 3 files changed, 7 insertions(+), 48 deletions(-)
>
> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c index
> 327011f..d1b9226 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c
> @@ -45,10 +45,10 @@
>
> #define MAX_ADDR_STR 32
>
> -static struct media *media_list = NULL;
> +static struct media media_list[MAX_MEDIA];
> static u32 media_count = 0;
>
> -struct bearer *tipc_bearers = NULL;
> +struct bearer tipc_bearers[MAX_BEARERS];
>
> /**
> * media_name_valid - validate media name @@ -660,26 +660,6
> @@ int tipc_disable_bearer(const char *name)
>
>
>
> -int tipc_bearer_init(void)
> -{
> - int res;
> -
> - write_lock_bh(&tipc_net_lock);
> - tipc_bearers = kcalloc(MAX_BEARERS, sizeof(struct
> bearer), GFP_ATOMIC);
> - media_list = kcalloc(MAX_MEDIA, sizeof(struct media),
> GFP_ATOMIC);
> - if (tipc_bearers && media_list) {
> - res = 0;
> - } else {
> - kfree(tipc_bearers);
> - kfree(media_list);
> - tipc_bearers = NULL;
> - media_list = NULL;
> - res = -ENOMEM;
> - }
> - write_unlock_bh(&tipc_net_lock);
> - return res;
> -}
> -
> void tipc_bearer_stop(void)
> {
> u32 i;
> @@ -695,10 +675,6 @@ void tipc_bearer_stop(void)
> if (tipc_bearers[i].active)
> bearer_disable(tipc_bearers[i].publ.name);
> }
> - kfree(tipc_bearers);
> - kfree(media_list);
> - tipc_bearers = NULL;
> - media_list = NULL;
> media_count = 0;
> }
>
> diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h index
> ca57348..000228e 100644
> --- a/net/tipc/bearer.h
> +++ b/net/tipc/bearer.h
> @@ -114,7 +114,7 @@ struct bearer_name {
>
> struct link;
>
> -extern struct bearer *tipc_bearers;
> +extern struct bearer tipc_bearers[];
>
> void tipc_media_addr_printf(struct print_buf *pb, struct
> tipc_media_addr *a); struct sk_buff
> *tipc_media_get_names(void); diff --git a/net/tipc/net.c
> b/net/tipc/net.c index 7906608..f25b1cd 100644
> --- a/net/tipc/net.c
> +++ b/net/tipc/net.c
> @@ -116,7 +116,8 @@
> */
>
> DEFINE_RWLOCK(tipc_net_lock);
> -struct network tipc_net = { NULL };
> +struct _zone *tipc_zones[256] = { NULL, }; struct network
> tipc_net = {
> +tipc_zones };
>
> struct tipc_node *tipc_net_select_remote_node(u32 addr, u32
> ref) { @@ -158,28 +159,12 @@ void
> tipc_net_send_external_routes(u32 dest)
> }
> }
>
> -static int net_init(void)
> -{
> - memset(&tipc_net, 0, sizeof(tipc_net));
> - tipc_net.zones = kcalloc(tipc_max_zones + 1,
> sizeof(struct _zone *), GFP_ATOMIC);
> - if (!tipc_net.zones) {
> - return -ENOMEM;
> - }
> - return 0;
> -}
> -
> static void net_stop(void)
> {
> u32 z_num;
>
> - if (!tipc_net.zones)
> - return;
> -
> - for (z_num = 1; z_num <= tipc_max_zones; z_num++) {
> + for (z_num = 1; z_num <= tipc_max_zones; z_num++)
> tipc_zone_delete(tipc_net.zones[z_num]);
> - }
> - kfree(tipc_net.zones);
> - tipc_net.zones = NULL;
> }
>
> static void net_route_named_msg(struct sk_buff *buf) @@
> -282,9 +267,7 @@ int tipc_net_start(u32 addr)
> tipc_named_reinit();
> tipc_port_reinit();
>
> - if ((res = tipc_bearer_init()) ||
> - (res = net_init()) ||
> - (res = tipc_cltr_init()) ||
> + if ((res = tipc_cltr_init()) ||
> (res = tipc_bclink_init())) {
> return res;
> }
>
------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
^ permalink raw reply
* Re: [RFC][PATCH] ns: Syscalls for better namespace sharing control.
From: Pavel Emelyanov @ 2010-03-03 16:50 UTC (permalink / raw)
To: Eric W. Biederman, Sukadev Bhattiprolu, Daniel Lezcano,
Serge Hallyn
Cc: Linux Netdev List, containers, Netfilter Development Mailinglist,
Ben Greear
In-Reply-To: <m1ocj6qljj.fsf@fess.ebiederm.org>
> I just played with this and if you make the semantics of unshare(CLONE_NEWPID)
> to be that you become the idle task aka pid 0, and not the init task pid 1 the
> implementation is trivial.
This is not ... handy - if after enter you have pid 0 you obviously
can't perform 2 parallel enters. The way I see it:
As far as the numbers reported to the userspace are concerned:
1. task, that enters is still visible by its old parent by old pid
2. task, that enters gets some pid within the entering namespace
and reports its parent pid to have pid 1 (init obviously doesn't
care)
3. we _can_ try to allocate new pid equal to the old one so that
glibc stays happy
As far as the pointers are concerned:
1. parent pointer doesn't change
2. task_pid(tsk) one (i.e. struct pid * one) _can_ change if
a) we don't allow threads enter (de_thread problem is handeled)
b) we don't allow leave the group/session, i.e. check, that there
is the only one task that enters lives in its pgid/sid
c) we wait for the quiescent state to pass by before destroying
the old pid to handle race with sys_kill()
Thoughts/questions? ("This is a nasty problem" answer is not acceptable,
the real code problems/races please)
^ permalink raw reply
* Re: [RFC][ PATCH 0/3] vhost-net: Add mergeable RX buffer support to vhost-net
From: David Stevens @ 2010-03-03 17:28 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, netdev, netdev-owner, rusty, virtualization
In-Reply-To: <20100303092850.GB14034@redhat.com>
> Interesting. Since the feature in question is billed first of all a
> performance optimization...
By whom? Although I see some improved performance, I think its real
benefit is improving memory utilization on the guest. Instead of using
75K for an ARP packet, mergeable RX buffers only uses 4K. :-)
> Since the patches affect code paths when mergeable RX buffers are
> disabled as well, I guess the most important point would be to verify
> whether there's increase in latency and/or CPU utilization, or bandwidth
> cost when the feature bit is *disabled*.
Actually, when the feature bit is disabled, it'll only get a single
head, doesn't use the special vnet_hdr, and the codepath reduces to the
essentially to the original. But the answer is "no"; I saw no regressions
when using it without the feature bit. The only substantive difference in
that case
is that the new code avoids copying the vnet header as the original
does, so it should actually be faster, but I don't think that's measurable
above the variability I already see.
>
> > 2 notes: I have a modified version of qemu to get the VHOST_FEATURES
> > flags, including the mergeable RX bufs flag, passed to the guest; I'll
> > be working with your current qemu git trees next, if any changes are
> > needed to support it there.
>
> This feature also seems to conflict with zero-copy rx patches from Xin
> Xiaohui (subject: Provide a zero-copy method on KVM virtio-net) these
> are not in a mergeable shape yet, so this is not a blocker, but I wonder
> what your thoughts on the subject are: how will we do feature
> negotiation if some backends don't support some features?
The qemu code I have basically sends the set features and get
features all the way to vhost (ie, it's the guest negotiating with
vhost), except, of course, for the magic qemu-only bits. I think that's
the right model. I'll definitely take a look at the patch you mention
and maybe comment further.
+-DLS
^ permalink raw reply
* Re: [net-next-2.6 PATCH] wireless: convert to use netdev_for_each_mc_addr
From: Jiri Pirko @ 2010-03-03 17:52 UTC (permalink / raw)
To: Jussi Kivilinna; +Cc: netdev, davem
In-Reply-To: <20100303184256.19122kg5hcqolfgg@hayate.sektori.org>
Wed, Mar 03, 2010 at 05:42:56PM CET, jussi.kivilinna@mbnet.fi wrote:
>Hello!
>
>Quoting "Jiri Pirko" <jpirko@redhat.com>:
>
>>+ } else if (mc_count) {
>>+ int size = min(priv->multicast_size, mc_count);
>>+ int i = 0;
>>+
>>+ mc_addrs = kmalloc(size * ETH_ALEN, GFP_ATOMIC);
>...
>>+ if (filter != basefilter)
>>+ goto set_filter;
>>+
>>+ if (mc_count) {
>>+ ret = rndis_set_oid(usbdev, OID_802_3_MULTICAST_LIST, mc_addrs,
>>+ mc_count * ETH_ALEN);
>>+ kfree(mc_addrs);
>
>mc_addrs was alloced by with 'size * ETH_ALEN', which might be less
>than mc_count * ETH_ALEN.
Actually it cannot. That's covered by:
if (mc_count > priv->multicast_size) {
This was also in the original code. In that case "size" can be eliminated and
"mc_addrs" can be allocated with "mc_count * ETH_ALEN".
Jussi are you ok with this?
Jirka
>
>Otherwise ok, and rndis_wlan works.
>
> -Jussi
>
^ permalink raw reply
* Re: [RFC v2 10/10] snet: introduce snet_utils
From: Jan Engelhardt @ 2010-03-03 17:55 UTC (permalink / raw)
To: Samir Bellabes
Cc: linux-security-module, linux-kernel, netdev, netfilter-devel,
jamal, Patrick McHardy, Evgeniy Polyakov, Neil Horman,
Grzegorz Nosek
In-Reply-To: <1267561394-13626-11-git-send-email-sam@synack.fr>
On Tuesday 2010-03-02 21:23, Samir Bellabes wrote:
>This patch provides helper functions for other subsystems
What subsystems would that be? (Just trying to prevent premature code
bloat.)
> security/snet/snet_utils.c | 38 ++++++++++++++++++++++++++++++++++++++
> security/snet/snet_utils.h | 10 ++++++++++
> 2 files changed, 48 insertions(+), 0 deletions(-)
> create mode 100644 security/snet/snet_utils.c
> create mode 100644 security/snet/snet_utils.h
>
>diff --git a/security/snet/snet_utils.c b/security/snet/snet_utils.c
>new file mode 100644
>index 0000000..e9178d7
>--- /dev/null
>+++ b/security/snet/snet_utils.c
>@@ -0,0 +1,38 @@
>+#include <linux/types.h>
>+#include <linux/snet.h>
>+
>+const char *snet_verdict_name(const enum snet_verdict cmd)
>+{
>+ static const char *const verdict_name[] = {
>+ [SNET_VERDICT_GRANT] = "Grant",
>+ [SNET_VERDICT_DENY] = "Deny",
>+ [SNET_VERDICT_PENDING] = "Pending",
>+ [SNET_VERDICT_NONE] = "None",
>+ };
>+
>+ if (cmd >= SNET_NR_VERDICT_TYPES)
>+ return "INVALID";
>+ else
>+ return verdict_name[cmd];
>+}
^ permalink raw reply
* Re: [net-next-2.6 PATCH] wireless: convert to use netdev_for_each_mc_addr
From: Jussi Kivilinna @ 2010-03-03 18:05 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem
In-Reply-To: <20100303175229.GB2760@psychotron.redhat.com>
Quoting "Jiri Pirko" <jpirko@redhat.com>:
> Wed, Mar 03, 2010 at 05:42:56PM CET, jussi.kivilinna@mbnet.fi wrote:
>> Hello!
>>
>> Quoting "Jiri Pirko" <jpirko@redhat.com>:
>>
>>> + } else if (mc_count) {
>>> + int size = min(priv->multicast_size, mc_count);
>>> + int i = 0;
>>> +
>>> + mc_addrs = kmalloc(size * ETH_ALEN, GFP_ATOMIC);
>> ...
>>> + if (filter != basefilter)
>>> + goto set_filter;
>>> +
>>> + if (mc_count) {
>>> + ret = rndis_set_oid(usbdev, OID_802_3_MULTICAST_LIST, mc_addrs,
>>> + mc_count * ETH_ALEN);
>>> + kfree(mc_addrs);
>>
>> mc_addrs was alloced by with 'size * ETH_ALEN', which might be less
>> than mc_count * ETH_ALEN.
>
> Actually it cannot. That's covered by:
>
> if (mc_count > priv->multicast_size) {
>
> This was also in the original code. In that case "size" can be eliminated and
> "mc_addrs" can be allocated with "mc_count * ETH_ALEN".
>
> Jussi are you ok with this?
>
> Jirka
Ah, you're right. Yes, 'size' can go away, it isn't needed after all.
I'm ok with this patch, I can fix 'size' to 'mc_count' myself later.
-Jussi
^ 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