* Re: [net-next-2.6 PATCH RFC] TCPCT part 1d: generate Responder Cookie
From: Eric Dumazet @ 2009-11-02 13:16 UTC (permalink / raw)
To: William Allen Simpson
Cc: Linux Kernel Developers, Linux Kernel Network Developers
In-Reply-To: <4AEED23A.7070009@gmail.com>
William Allen Simpson a écrit :
> Eric Dumazet wrote:
>> cookie_hash() runs in a non preemptable context. CPU cannot change
>> under us.
>>
>> (or else, we would not use __get_cpu_var(ipv4_cookie_scratch); )
>>
>> And of course, each cpu gets its own scratch area, thanks to
>> __get_cpu_var()
>>
> Interesting. I'm not sure that running CPU intensive functions like
> SHA1 in
> a non-preemptable context is a good idea. I'd assumed it wasn't!
>
> Perhaps you could point at the documentation in the code that explains
> this?
I suggest you read Documentations/ files about softirq
http://docs.blackfin.uclinux.org/kernel/generated/kernel-hacking.xml
Large part of network code is run by softirq handler, and a softirq handler
is not preemptable with another softirq (including itself).
> Perhaps a function header comment that mentions it?
So we are going to add a header to thousand of functions repeating this prereq ?
>
> All I know is (from testing) that the tcp_minisockets.c caller is sometimes
> called in a fashion that requires atomic allocation, and other times
> does not!
Maybe callers have different contexts (running from softirq handler or
from process context). Atomic ops are expensive and we try to avoid them
if/when possible.
>
> See my "Subject: query: tcpdump versus atomic?" thread from Oct 14th.
You probably add a bug in your kernel, leaving a function with unpaired lock/unlock
of notallow_something/allow_something
There are books about linux internals that you could read if you want some extra
documentation. Dont ask me details, I never read them :)
^ permalink raw reply
* Re: [RFC] multiqueue changes
From: Jarek Poplawski @ 2009-11-02 13:09 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, mchan, kaber, netdev
In-Reply-To: <4AEED899.9040106@gmail.com>
On Mon, Nov 02, 2009 at 02:03:21PM +0100, Eric Dumazet wrote:
...
> Sure, but I prefer a patch from you ;)
>
Wrong code! (RFP? ;-)
Jarek P.
^ permalink raw reply
* Re: [RFC] multiqueue changes
From: Eric Dumazet @ 2009-11-02 13:03 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: David Miller, mchan, kaber, netdev
In-Reply-To: <20091102130223.GB7790@ff.dom.local>
Jarek Poplawski a écrit :
> On Mon, Nov 02, 2009 at 04:39:07AM -0800, David Miller wrote:
>> From: Jarek Poplawski <jarkao2@gmail.com>
>> Date: Mon, 2 Nov 2009 12:30:29 +0000
>>
>>> Right, but it's not a 50% chance, I guess? A user most of the time
>>> gets consistently multiqueue or non-multiqueue behavior after open,
>>> unless I miss something. Then such an exceptional state could be
>>> handled by real_num_tx_queues (just like in case of powered of cpus).
>>> The main difference is to hold in num_tx_queues something that is
>>> really available vs max possible value for all configs.
>> I see your point, yes this would seem to be a reasonable way
>> to start handling num_tx_queues and real_num_tx_queues.
>
> Very nice! So, I hope Eric should be satisfied with these requested
> comments already :-)
>
Sure, but I prefer a patch from you ;)
^ permalink raw reply
* Re: [RFC] multiqueue changes
From: Jarek Poplawski @ 2009-11-02 13:02 UTC (permalink / raw)
To: David Miller; +Cc: mchan, kaber, eric.dumazet, netdev
In-Reply-To: <20091102.043907.236634594.davem@davemloft.net>
On Mon, Nov 02, 2009 at 04:39:07AM -0800, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Mon, 2 Nov 2009 12:30:29 +0000
>
> > Right, but it's not a 50% chance, I guess? A user most of the time
> > gets consistently multiqueue or non-multiqueue behavior after open,
> > unless I miss something. Then such an exceptional state could be
> > handled by real_num_tx_queues (just like in case of powered of cpus).
> > The main difference is to hold in num_tx_queues something that is
> > really available vs max possible value for all configs.
>
> I see your point, yes this would seem to be a reasonable way
> to start handling num_tx_queues and real_num_tx_queues.
Very nice! So, I hope Eric should be satisfied with these requested
comments already :-)
Thanks everybody,
Jarek P.
^ permalink raw reply
* Re: [net-next-2.6 PATCH v4 3/3] TCPCT part 1c: initial SYN exchange with SYNACK data
From: Ilpo Järvinen @ 2009-11-02 12:57 UTC (permalink / raw)
To: William Allen Simpson; +Cc: Eric Dumazet, Linux Kernel Network Developers
In-Reply-To: <4AEECFA8.1080306@gmail.com>
On Mon, 2 Nov 2009, William Allen Simpson wrote:
> Eric Dumazet wrote:
> > This part is really hard to review, and might be splitted ?
> >
> > cleanups could be done in a cleanup patch only
> >
> > Examples:
> >
> > - tmp_opt.mss_clamp = 536;
> > - tmp_opt.user_mss = tcp_sk(sk)->rx_opt.user_mss;
> > + tmp_opt.mss_clamp = TCP_MIN_RCVMSS;
> > + tmp_opt.user_mss = tp->rx_opt.user_mss;
> >
> >
> > - tp->mss_cache = 536;
> > + tp->mss_cache = TCP_MIN_RCVMSS;
> >
> Often hard to decide what's "cleanup" and what's essential. I'll look at
> that again for the next round, but I've already split the original single
> patch into multiple parts.
Are you talking about particular case?!? ...You can safely split into even
more parts if there are cleanups which is essential. ...We'll not stop you
from doing that nor be angry if do that.
> > Also your tests are reversed, if you look at the existing coding style.
> >
> I checked Documentation/CodingStyle, and that's not specified. I've seen
> plenty of examples of modern security coding style around here.
>
> As a long-time (25+ years) consultant and 30 years C programmer, I'm
> heedful of the project coding style, and had to endure many variants.
>
> Where I'm working with others' code, you'll note that I keep the same
> style, no matter how ugly, as that makes patches easier to read.
>
>
> > Example :
> >
> > + /* TCP Cookie Transactions */
> > + if (0 < sysctl_tcp_cookie_size) {
> > + /* Default, cookies without s_data. */
> > + tp->cookie_values =
> > + kzalloc(sizeof(*tp->cookie_values),
> > + sk->sk_allocation);
> > + if (NULL != tp->cookie_values)
> > + kref_init(&tp->cookie_values->kref);
> > + }
> >
> > should be ->
> >
> > + /* TCP Cookie Transactions */
> > + if (sysctl_tcp_cookie_size > 0) {
> > + /* Default, cookies without s_data. */
> > + tp->cookie_values =
> > + kzalloc(sizeof(*tp->cookie_values),
> > + sk->sk_allocation);
> > + if (tp->cookie_values != NULL)
> > + kref_init(&tp->cookie_values->kref);
> > + }
> >
> And "tp->cookie_values != NULL" is egregiously poor C practice. It's very
> hard for code review to ensure that didn't get truncated to "= NULL". The
> important visual element is the NULL, not the variable name.
>
> Also, avoid "!tp->cookie_values", as this is *not* a boolean.
>
> When I'm adding new code, I use constant-to-the-left security coding style,
> as they teach in modern universities (lately also for PHP). And this is a
> security extension, so a security style is particularly appropriate.
>
> As in switch statements, constant-to-the-left makes the value obvious,
> especially in a series (and assists transforming if series into a switch).
>
> For complex tests, this makes the code much more readable and easier to
> visually verify on code walk-through:
>
> + if (0 < tmp_opt.cookie_plus
> + && tmp_opt.saw_tstamp
> + && !tp->cookie_out_never
> + && (0 < sysctl_tcp_cookie_size
> + || (NULL != tp->cookie_values
> + && 0 < tp->cookie_values->cookie_desired))) {
>
> Consistent use of security style would have obviated a lot of foolish >= 0
> tests that seem to be constantly in need of fixing. It's a bad idea to
> depend on the compiler to catch non-executable code.
That kind of response certainly won't help you any. ...First, you said you
adapt the current style but for some reason immediately start to say why
you would careless about that principle. ...Also, telling that you have
lots of experience here and there will not get you there either ;-).
--
i.
^ permalink raw reply
* Re: [RFC] multiqueue changes
From: David Miller @ 2009-11-02 12:39 UTC (permalink / raw)
To: jarkao2; +Cc: mchan, kaber, eric.dumazet, netdev
In-Reply-To: <20091102123029.GA7790@ff.dom.local>
From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 2 Nov 2009 12:30:29 +0000
> Right, but it's not a 50% chance, I guess? A user most of the time
> gets consistently multiqueue or non-multiqueue behavior after open,
> unless I miss something. Then such an exceptional state could be
> handled by real_num_tx_queues (just like in case of powered of cpus).
> The main difference is to hold in num_tx_queues something that is
> really available vs max possible value for all configs.
I see your point, yes this would seem to be a reasonable way
to start handling num_tx_queues and real_num_tx_queues.
^ permalink raw reply
* Re: [net-next-2.6 PATCH RFC] TCPCT part 1d: generate Responder Cookie
From: William Allen Simpson @ 2009-11-02 12:36 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Linux Kernel Developers, Linux Kernel Network Developers
In-Reply-To: <4AEEBAC6.7020308@gmail.com>
Eric Dumazet wrote:
> cookie_hash() runs in a non preemptable context. CPU cannot change under us.
>
> (or else, we would not use __get_cpu_var(ipv4_cookie_scratch); )
>
> And of course, each cpu gets its own scratch area, thanks to __get_cpu_var()
>
Interesting. I'm not sure that running CPU intensive functions like SHA1 in
a non-preemptable context is a good idea. I'd assumed it wasn't!
Perhaps you could point at the documentation in the code that explains this?
Perhaps a function header comment that mentions it?
All I know is (from testing) that the tcp_minisockets.c caller is sometimes
called in a fashion that requires atomic allocation, and other times does not!
See my "Subject: query: tcpdump versus atomic?" thread from Oct 14th.
^ permalink raw reply
* Re: [RFC] multiqueue changes
From: Jarek Poplawski @ 2009-11-02 12:30 UTC (permalink / raw)
To: David Miller; +Cc: mchan, kaber, eric.dumazet, netdev
In-Reply-To: <20091102.033533.08766686.davem@davemloft.net>
On Mon, Nov 02, 2009 at 03:35:33AM -0800, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Sun, 1 Nov 2009 14:20:17 +0100
>
> > There is a question if we can predict in ->probe() MSI-X should be
> > successfully enabled in ->ndo_open() for probed hardware. If so,
> > then it could go e.g. like this:
>
> We never can know this.
>
> Another device driver can eat up all the MSI-X vectors in the PCI
> domain before we make the request_irq() calls in ->open().
Right, but it's not a 50% chance, I guess? A user most of the time
gets consistently multiqueue or non-multiqueue behavior after open,
unless I miss something. Then such an exceptional state could be
handled by real_num_tx_queues (just like in case of powered of cpus).
The main difference is to hold in num_tx_queues something that is
really available vs max possible value for all configs.
Jarek P.
^ permalink raw reply
* Re: [net-next PATCH 0/4] qlge: Add ethtool self-test.
From: David Miller @ 2009-11-02 12:26 UTC (permalink / raw)
To: ron.mercer; +Cc: netdev
In-Reply-To: <1256940816-27540-1-git-send-email-ron.mercer@qlogic.com>
From: Ron Mercer <ron.mercer@qlogic.com>
Date: Fri, 30 Oct 2009 15:13:32 -0700
> Changes for QLGE.
>
> 1) Add ethtool selftest.
> 2) Reduce debug print output.
> 3) Generic cleanup.
I've applied patches #1 and #2
Patch #3 needs to be looked into based upon the feedback we
gave you.
Patch #4 needs to be respun based upon whatever happens to
patch #3.
Thanks Ron.
^ permalink raw reply
* Re: [net-next-2.6 PATCH v4 3/3] TCPCT part 1c: initial SYN exchange with SYNACK data
From: William Allen Simpson @ 2009-11-02 12:25 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Linux Kernel Network Developers
In-Reply-To: <4AEDDF33.9030205@gmail.com>
Eric Dumazet wrote:
> This part is really hard to review, and might be splitted ?
>
> cleanups could be done in a cleanup patch only
>
> Examples:
>
> - tmp_opt.mss_clamp = 536;
> - tmp_opt.user_mss = tcp_sk(sk)->rx_opt.user_mss;
> + tmp_opt.mss_clamp = TCP_MIN_RCVMSS;
> + tmp_opt.user_mss = tp->rx_opt.user_mss;
>
>
> - tp->mss_cache = 536;
> + tp->mss_cache = TCP_MIN_RCVMSS;
>
Often hard to decide what's "cleanup" and what's essential. I'll look at
that again for the next round, but I've already split the original single
patch into multiple parts.
> Also your tests are reversed, if you look at the existing coding style.
>
I checked Documentation/CodingStyle, and that's not specified. I've seen
plenty of examples of modern security coding style around here.
As a long-time (25+ years) consultant and 30 years C programmer, I'm
heedful of the project coding style, and had to endure many variants.
Where I'm working with others' code, you'll note that I keep the same
style, no matter how ugly, as that makes patches easier to read.
> Example :
>
> + /* TCP Cookie Transactions */
> + if (0 < sysctl_tcp_cookie_size) {
> + /* Default, cookies without s_data. */
> + tp->cookie_values =
> + kzalloc(sizeof(*tp->cookie_values),
> + sk->sk_allocation);
> + if (NULL != tp->cookie_values)
> + kref_init(&tp->cookie_values->kref);
> + }
>
> should be ->
>
> + /* TCP Cookie Transactions */
> + if (sysctl_tcp_cookie_size > 0) {
> + /* Default, cookies without s_data. */
> + tp->cookie_values =
> + kzalloc(sizeof(*tp->cookie_values),
> + sk->sk_allocation);
> + if (tp->cookie_values != NULL)
> + kref_init(&tp->cookie_values->kref);
> + }
>
And "tp->cookie_values != NULL" is egregiously poor C practice. It's very
hard for code review to ensure that didn't get truncated to "= NULL". The
important visual element is the NULL, not the variable name.
Also, avoid "!tp->cookie_values", as this is *not* a boolean.
When I'm adding new code, I use constant-to-the-left security coding style,
as they teach in modern universities (lately also for PHP). And this is a
security extension, so a security style is particularly appropriate.
As in switch statements, constant-to-the-left makes the value obvious,
especially in a series (and assists transforming if series into a switch).
For complex tests, this makes the code much more readable and easier to
visually verify on code walk-through:
+ if (0 < tmp_opt.cookie_plus
+ && tmp_opt.saw_tstamp
+ && !tp->cookie_out_never
+ && (0 < sysctl_tcp_cookie_size
+ || (NULL != tp->cookie_values
+ && 0 < tp->cookie_values->cookie_desired))) {
Consistent use of security style would have obviated a lot of foolish >= 0
tests that seem to be constantly in need of fixing. It's a bad idea to
depend on the compiler to catch non-executable code.
^ permalink raw reply
* Re: [PATCH net-next-2.6] ipv6: no more dev_put() in datagram_send_ctl()
From: David Miller @ 2009-11-02 11:43 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <4AEEC0A2.1070502@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Nov 2009 12:21:06 +0100
> Avoids touching device refcount in datagram_send_ctl(), thanks to RCU
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next-2.6] ipv6: no more dev_put() in inet6_bind()
From: David Miller @ 2009-11-02 11:43 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <4AEEBE2F.4020005@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Nov 2009 12:10:39 +0100
> Avoids touching device refcount in inet6_bind(), thanks to RCU
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next-2.6] ip6tnl: less dev_put() calls
From: David Miller @ 2009-11-02 11:43 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <4AEEB2B1.9070302@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Nov 2009 11:21:37 +0100
> Using dev_get_by_index_rcu() in ip6_tnl_rcv_ctl() & ip6_tnl_xmit_ctl()
> avoids touching device refcount.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next-2.6] packet: less dev_put() calls
From: David Miller @ 2009-11-02 11:42 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <4AEEA9C4.6040305@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 02 Nov 2009 10:43:32 +0100
> - packet_sendmsg_spkt() can use dev_get_by_name_rcu() to avoid touching device refcount.
>
> - packet_getname_spkt() & packet_getname() can use dev_get_by_index_rcu() to
> avoid touching device refcount too.
>
> tpacket_snd() & packet_snd() can not use RCU yet because they can sleep when
> allocating skb.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
You're unstoppable :-)
Applied.
^ permalink raw reply
* Re: [RFC] multiqueue changes
From: David Miller @ 2009-11-02 11:35 UTC (permalink / raw)
To: jarkao2; +Cc: mchan, kaber, eric.dumazet, netdev
In-Reply-To: <20091101132017.GA2598@ami.dom.local>
From: Jarek Poplawski <jarkao2@gmail.com>
Date: Sun, 1 Nov 2009 14:20:17 +0100
> There is a question if we can predict in ->probe() MSI-X should be
> successfully enabled in ->ndo_open() for probed hardware. If so,
> then it could go e.g. like this:
We never can know this.
Another device driver can eat up all the MSI-X vectors in the PCI
domain before we make the request_irq() calls in ->open().
^ permalink raw reply
* Re: Patch: Fix userland build failure in linux/if_tunnel.h
From: David Miller @ 2009-11-02 11:30 UTC (permalink / raw)
To: sposelenov; +Cc: sledz, sledz, wd, linux-kernel, netdev
In-Reply-To: <20091102135750.436a7b54@emcraft.com>
From: Sergei Poselenov <sposelenov@emcraft.com>
Date: Mon, 2 Nov 2009 13:57:50 +0300
> Hm, I received a confirmation, that my fix was applied
> http://www.spinics.net/lists/netdev/msg103860.html
>
> However, it indeed didn't find their way into the mainline.
> I have no idea why, received no objections.
That posting doesn't say that your patch got applied, it says
that Joe Perches's MAINTAINERS file patch got applied.
Look at the context of what I'm replying to, rather than the
fact that you see "Applied." and it happens to be in a thread
you started with another patch.
Your patch is not necessary, current iproute2 sources compile
just fine without your change. iproute2 got changed to include
a local copy of linux/ip_tunnel.h which is modified so that it
compiles properly in userspace.
^ permalink raw reply
* [PATCH net-next-2.6] ipv6: no more dev_put() in datagram_send_ctl()
From: Eric Dumazet @ 2009-11-02 11:21 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
Avoids touching device refcount in datagram_send_ctl(), thanks to RCU
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/ipv6/datagram.c | 14 +++++++++-----
1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index 9f70452..e6f9cdf 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -537,12 +537,17 @@ int datagram_send_ctl(struct net *net,
addr_type = __ipv6_addr_type(&src_info->ipi6_addr);
+ rcu_read_lock();
if (fl->oif) {
- dev = dev_get_by_index(net, fl->oif);
- if (!dev)
+ dev = dev_get_by_index_rcu(net, fl->oif);
+ if (!dev) {
+ rcu_read_unlock();
return -ENODEV;
- } else if (addr_type & IPV6_ADDR_LINKLOCAL)
+ }
+ } else if (addr_type & IPV6_ADDR_LINKLOCAL) {
+ rcu_read_unlock();
return -EINVAL;
+ }
if (addr_type != IPV6_ADDR_ANY) {
int strict = __ipv6_addr_src_scope(addr_type) <= IPV6_ADDR_SCOPE_LINKLOCAL;
@@ -553,8 +558,7 @@ int datagram_send_ctl(struct net *net,
ipv6_addr_copy(&fl->fl6_src, &src_info->ipi6_addr);
}
- if (dev)
- dev_put(dev);
+ rcu_read_unlock();
if (err)
goto exit_f;
^ permalink raw reply related
* Re: Patch: Fix userland build failure in linux/if_tunnel.h
From: Sergei Poselenov @ 2009-11-02 10:57 UTC (permalink / raw)
To: Steffen Sledz, Steffen Sledz; +Cc: sledz, wd, linux-kernel, netdev
In-Reply-To: <4AEDCBA6.7040206@dresearch.de>
Hello Steffen,
Hm, I received a confirmation, that my fix was applied
http://www.spinics.net/lists/netdev/msg103860.html
However, it indeed didn't find their way into the mainline.
I have no idea why, received no objections.
I'm sending the reply to the resp. mailing lists too; hope you don't
mind.
Regards,
Sergei
On Sun, 01 Nov 2009 18:55:50 +0100
Steffen Sledz <sledz@dresearch.de> wrote:
> Hello Sergei,
>
> i'm working for the OpenEmbedded project and we hit the same busybox
> problem you mentioned in <http://patchwork.kernel.org/patch/31164/>.
>
> It doesn't look like you got much feedback on you patch suggestion.
> Did it made it's way to the kernel sources? Or where there any
> objections on it?
>
> Steffen
^ permalink raw reply
* RE: Patch: Fix userland build failure in linux/if_tunnel.h
From: Sledz, Steffen @ 2009-11-02 11:15 UTC (permalink / raw)
To: Sergei Poselenov; +Cc: wd, linux-kernel, netdev
In-Reply-To: <20091102135750.436a7b54@emcraft.com>
Hello Sergei, hello other maintainers,
> > i'm working for the OpenEmbedded project and we hit the same busybox
> > problem you mentioned in <http://patchwork.kernel.org/patch/31164/>.
> >
> > It doesn't look like you got much feedback on you patch suggestion.
> > Did it made it's way to the kernel sources? Or where there any
> > objections on it?
> Hm, I received a confirmation, that my fix was applied
> http://www.spinics.net/lists/netdev/msg103860.html
No, i think it didn't. This confirmation is related to a patch from
Joe Perches to the MAINTAINERS file.
> However, it indeed didn't find their way into the mainline.
> I have no idea why, received no objections.
What are the maintainers saying?
Steffen
^ permalink raw reply
* [PATCH net-next-2.6] ipv6: no more dev_put() in inet6_bind()
From: Eric Dumazet @ 2009-11-02 11:10 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
Avoids touching device refcount in inet6_bind(), thanks to RCU
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/ipv6/af_inet6.c | 17 +++++++++--------
1 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index b6d0588..9105b25 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -314,6 +314,7 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
if (addr_type != IPV6_ADDR_ANY) {
struct net_device *dev = NULL;
+ rcu_read_lock();
if (addr_type & IPV6_ADDR_LINKLOCAL) {
if (addr_len >= sizeof(struct sockaddr_in6) &&
addr->sin6_scope_id) {
@@ -326,12 +327,12 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
/* Binding to link-local address requires an interface */
if (!sk->sk_bound_dev_if) {
err = -EINVAL;
- goto out;
+ goto out_unlock;
}
- dev = dev_get_by_index(net, sk->sk_bound_dev_if);
+ dev = dev_get_by_index_rcu(net, sk->sk_bound_dev_if);
if (!dev) {
err = -ENODEV;
- goto out;
+ goto out_unlock;
}
}
@@ -342,14 +343,11 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
if (!(addr_type & IPV6_ADDR_MULTICAST)) {
if (!ipv6_chk_addr(net, &addr->sin6_addr,
dev, 0)) {
- if (dev)
- dev_put(dev);
err = -EADDRNOTAVAIL;
- goto out;
+ goto out_unlock;
}
}
- if (dev)
- dev_put(dev);
+ rcu_read_unlock();
}
}
@@ -381,6 +379,9 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
out:
release_sock(sk);
return err;
+out_unlock:
+ rcu_read_unlock();
+ goto out;
}
EXPORT_SYMBOL(inet6_bind);
^ permalink raw reply related
* Re: [net-next-2.6 PATCH RFC] TCPCT part 1d: generate Responder Cookie
From: Eric Dumazet @ 2009-11-02 10:56 UTC (permalink / raw)
To: William Allen Simpson
Cc: Linux Kernel Developers, Linux Kernel Network Developers
In-Reply-To: <4AEEB6D2.7090505@gmail.com>
William Allen Simpson a écrit :
>
> The (buggy?) syncookie code that I'm avoiding/replacing is:
>
> static DEFINE_PER_CPU(__u32 [16 + 5 + SHA_WORKSPACE_WORDS],
> ipv4_cookie_scratch);
>
> static u32 cookie_hash(__be32 saddr, __be32 daddr, __be16 sport, __be16
> dport,
> u32 count, int c)
> {
> __u32 *tmp = __get_cpu_var(ipv4_cookie_scratch);
>
> memcpy(tmp + 4, syncookie_secret[c], sizeof(syncookie_secret[c]));
> tmp[0] = (__force u32)saddr;
> tmp[1] = (__force u32)daddr;
> tmp[2] = ((__force u32)sport << 16) + (__force u32)dport;
> tmp[3] = count;
> sha_transform(tmp + 16, (__u8 *)tmp, tmp + 16 + 5);
>
> return tmp[17];
> }
>
> It appears to me that the operations could be interrupted at any time, and
> the fairly expensive sha transform (or probably any of the assignments)
> could be corrupted?
>
> That is, cpu0 grabs a scratch area, copies some data into it, interrupts,
> cpu0 comes along again with another packet, points into the same workspace,
> mashes the data, while cpu1 completes the previous operation with the
> old tmp pointer on the stack.
>
> Worse, this is called twice, each time getting tmp, and mashing the data,
> and at the same time others are calling it twice more for verification.
>
> Since syncookies only occur under stress, the code isn't well tested, and
> the only symptom would be the returned packet would be dropped after
> failing to verify. Since this only happens when lots of packets are
> arriving, dropped packets probably aren't noticed.
>
> However, that would be unacceptable for my code.
>
cookie_hash() runs in a non preemptable context. CPU cannot change under us.
(or else, we would not use __get_cpu_var(ipv4_cookie_scratch); )
And of course, each cpu gets its own scratch area, thanks to __get_cpu_var()
^ permalink raw reply
* Re: [net-next-2.6 PATCH v4 2/3] TCPCT part 1b: sysctl_tcp_cookie_size, socket option TCP_COOKIE_TRANSACTIONS, functions
From: David Miller @ 2009-11-02 10:51 UTC (permalink / raw)
To: william.allen.simpson; +Cc: eric.dumazet, netdev
In-Reply-To: <4AEEB842.1070705@gmail.com>
From: William Allen Simpson <william.allen.simpson@gmail.com>
Date: Mon, 02 Nov 2009 05:45:22 -0500
> Is that a problem?
Yes, it is a problem.
You can allocate the bit when you add the feature.
^ permalink raw reply
* Re: [net-next-2.6 PATCH RFC] TCPCT part 1d: generate Responder Cookie
From: David Miller @ 2009-11-02 10:50 UTC (permalink / raw)
To: william.allen.simpson; +Cc: eric.dumazet, linux-kernel, netdev
In-Reply-To: <4AEEB6D2.7090505@gmail.com>
From: William Allen Simpson <william.allen.simpson@gmail.com>
Date: Mon, 02 Nov 2009 05:39:14 -0500
> Thank you for your helpful comments. I'm not familiar with LKS. Is
> that a month long project?
It's the Linux Kernel Summit. It's well publicized:
http://events.linuxfoundation.org/events/kernel-summit
It's the yearly meeting of the top Linux kernel developers, it usually
lasts several days and since a large portion of the kernel developers
who attend have to travel on 10+ hour flights to reach the summit
location most choose to pad their trip on either side by several days
in order to recover from jet lag and other effects of travelling.
This year most kernel developers stayed for the rest of the week in
order to attend the Japan Linux Symposium.
Many people were offline a total of 2 weeks or more because of these
events. Some still haven't returned from Japan.
But that's neither here nor there, people sometimes are simply
overloaded with work that is important to them, or from their
view more important than reviewing your work.
Sometimes that happens and you just have to be patient and
understanding.
Complaining that your work isn't getting looked at in a timely manner
will always have the exact opposite effect that you want, it makes
people have a smaller desire to look at your stuff.
^ permalink raw reply
* Re: [net-next-2.6 PATCH v4 2/3] TCPCT part 1b: sysctl_tcp_cookie_size, socket option TCP_COOKIE_TRANSACTIONS, functions
From: William Allen Simpson @ 2009-11-02 10:45 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Linux Kernel Network Developers
In-Reply-To: <4AEDDDF3.3070102@gmail.com>
Eric Dumazet wrote:
> William Allen Simpson a écrit :
>> These functions will also be used in subsequent patches that implement
>> additional features.
>>
>
> +#define TCP_EXTEND_TIMESTAMP (1 << 4) /* Initiate 64-bit timestamps */
>
> What is this for ?
>
Reserving a bit in the sockopt for a future feature (described in the
aforementioned end2end-interest email messages).
Certainly, reserving bits seems to be common elsewhere in Linux code. I'm
trying to coordinate this with the *BSD developers, too, so it helps to keep
API documentation synchronized. Is that a problem?
^ permalink raw reply
* Re: [net-next-2.6 PATCH RFC] TCPCT part 1d: generate Responder Cookie
From: William Allen Simpson @ 2009-11-02 10:39 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Linux Kernel Developers, Linux Kernel Network Developers
In-Reply-To: <4AEDCD7C.2010403@gmail.com>
Eric Dumazet wrote:
> William Allen Simpson a écrit :
>> Since October 4th, I've repeatedly asked publicly for assistance with these
>> Linux-specific memory locking constructs and cryptography. I've also sent
>> private messages. No help has been forthcoming. None. Nada.
>>
>> At this point, I've spent weeks re-spinning code that I'd understood was
>> approved a year ago. The whole project should have been finished by now!
>
> Your messages on netdev are two weeks old, not one year, and came during
> LKS. Many developpers were busy in Japan.
>
Thank you for your helpful comments. I'm not familiar with LKS. Is that a
month long project?
The message in question was "query: per cpu hash pool spinlock", sent:
"Sun, 04 Oct 2009 06:37:39 -0400".
My initial query for this series was "query: adding a sysctl", sent:
"Fri, 02 Oct 2009 00:00:05 -0400".
The earlier RFC approval was circa 1 year, 11 weeks, 2 days, 23 hours ago:
http://article.gmane.org/gmane.linux.network/102779
>> So, I'll try a larger audience. Could somebody take a look at my usage of
>> read and write locking?
>>
>> NB, I'm trying to port some 15-year-old fairly simple and straightforward
>> (single cpu) code from the KA9Q cooperative multitasking platform.
>>
>> I've examined existing code used for syncookies and TCP MD5 authenticators.
>> Neither meets my needs, as this secret is updated every few minutes. Both
>> have very different approaches. They are rarely used. My code will be
>> used on the order of tens of thousands of connections per second.
>>
>> Moreover, it seems to my naive eye that the syncookie per cpu code simply
>> doesn't work properly. The workspace is allocated per cpu, but the cpu
>> could change during the extensive SHA1 computations. Bug?
>>
The (buggy?) syncookie code that I'm avoiding/replacing is:
static DEFINE_PER_CPU(__u32 [16 + 5 + SHA_WORKSPACE_WORDS],
ipv4_cookie_scratch);
static u32 cookie_hash(__be32 saddr, __be32 daddr, __be16 sport, __be16 dport,
u32 count, int c)
{
__u32 *tmp = __get_cpu_var(ipv4_cookie_scratch);
memcpy(tmp + 4, syncookie_secret[c], sizeof(syncookie_secret[c]));
tmp[0] = (__force u32)saddr;
tmp[1] = (__force u32)daddr;
tmp[2] = ((__force u32)sport << 16) + (__force u32)dport;
tmp[3] = count;
sha_transform(tmp + 16, (__u8 *)tmp, tmp + 16 + 5);
return tmp[17];
}
It appears to me that the operations could be interrupted at any time, and
the fairly expensive sha transform (or probably any of the assignments)
could be corrupted?
That is, cpu0 grabs a scratch area, copies some data into it, interrupts,
cpu0 comes along again with another packet, points into the same workspace,
mashes the data, while cpu1 completes the previous operation with the
old tmp pointer on the stack.
Worse, this is called twice, each time getting tmp, and mashing the data,
and at the same time others are calling it twice more for verification.
Since syncookies only occur under stress, the code isn't well tested, and
the only symptom would be the returned packet would be dropped after
failing to verify. Since this only happens when lots of packets are
arriving, dropped packets probably aren't noticed.
However, that would be unacceptable for my code.
> This patch looks fine, but I dont see how this new function is used.
>
It is used at the places in the previous numbered patch (part 1c) that
currently say, "secret recipe not yet implemented".
And perhaps to replace the syncookie code mentioned above.
> Some points :
>
> 1) We are working hard to remove rwlocks from network stack, so please dont
> add a new one.
Botheration! I wish that was *documented* somewhere, perhaps in:
Documentation/spinlocks.txt
> add a new one. You probably can use a seqlock or RCU, or a server handling
> 10.000 connections request per second on many NIC will hit this rwlock.
>
Yep. Nobody in the tcp code is using them, but RCU looks interesting, and
may be well suited. I'll read up on it.
> 2)
>
> } else if (unlikely(time_after(jiffy, tcp_secret_primary->expires))) {
> get_random_bytes(secrets, sizeof(secrets));
>
> write_lock(&tcp_secret_locker);
>
> It would be better to first get the lock, then get random_bytes, in order
> not wasting entropy.
>
I'd put that outside the locked section as it's considerably more expensive
(hashing) than a memcpy. Entropy is never "wasted", as good cryptographic
random functions never reveal underlying entropy. I know there were some
papers a few years ago that found flaws in Linux random functions, but
surely that's been fixed. I assume you folks are using Yarrow or its
successors by now.
In any case, perhaps this RCU thingy obviates that issue.
>
> 3) If you change secret ever 600 seconds, it might be better to use a timer
> so that you dont have to check expiration and this logic at each SYN packet.
> (Disociate the lookup (read-only, done many time per second) from the updates
> (trigerred by a timer every 600 secs))
>
Part of the logic is not to update the generation secret until a new packet
arrives, allowing maximal entropy to be gathered beforehand. This is
especially worrisome for the first packets after booting. It takes into
account inter-packet arrival timing, and expiring after MSL. So, a timer
isn't feasible in this instance.
> (Not counting you'll probably need to use a similar lookup algo for the ACK
> packet coming from client)
>
Yes. A timer could be used to expire and clear old verification secrets,
and I'll look into that in the future ACK patch.
Thanks again.
^ 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