* Re: [PATCH 4/10] Fix leaking of kernel heap addresses in net/
From: David Miller @ 2010-11-12 2:29 UTC (permalink / raw)
To: drosenberg-PiUznwcHFHrqlBn2x/YWAg
Cc: urs.thuermann-l29pVbxQd1IUtdQbppsyvg,
socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
linux-sctp-u79uwXL29TY76Z2rM5mHXA,
shemminger-ZtmgI6mnKB3QT0dZR+AlfA, xemul-GEFAQzZX7r8dnm+yROfE0A,
pekkas-UjJjq++bwZ7HOG6cAo2yLw,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
jmorris-gx6/JNMH7DfYtjvyW6yDsg, kuznet-v/Mj1YrvjDBInbfyfbPRSQ,
adobriyan-Re5JQEeQqe8AvxtiuMwx3w, sri-r/Jw6+rmf7HQT0dZR+AlfA,
johannes.berg-ral2JQCrhuEAvxtiuMwx3w, hadi-jkUAjuhPggJWk0Htik3J/w,
vladislav.yasevich-VXdhtT5mjnY, lizf-BthXqXjhjHXQFUHtdCDX3A,
tj-DgEjT+Ai2ygdnm+yROfE0A,
remi.denis-courmont-xNZwKgViW5gAvxtiuMwx3w,
daniel.lezcano-GANU6spQydw, jpirko-H+wXaHxf7aLQT0dZR+AlfA,
yoshfuji-VfPWfsRibaP+Ru+s062T9g, socketcan-fJ+pQTUTwRTk1uMJSBkQmQ,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, netdev-u79uwXL29TY76Z2rM5mHXA,
joe-6d6DIl74uiNBDgjK7y7TUQ,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
kaber-dcUjhNyLwpNeoWH0uzbU5w
In-Reply-To: <2129857903-1289528127-cardhu_decombobulator_blackberry.rim.net-1506931048--JnVBb1XAImjjL2gL5RxOEzYg3SYOavFBmZ6FRVpaDsI@public.gmane.org>
From: "Dan Rosenberg" <drosenberg-PiUznwcHFHrqlBn2x/YWAg@public.gmane.org>
Date: Fri, 12 Nov 2010 02:15:25 +0000
> I am willing to do whatever is necessary to make these changes
> acceptable. Are you completely set on not removing these addresses
> from the output?
I want whatever you replace it with to be equivalent for
object tracking purposes.
^ permalink raw reply
* Re: [PATCH 4/10] Fix leaking of kernel heap addresses in net/
From: Dan Rosenberg @ 2010-11-12 2:34 UTC (permalink / raw)
To: David Miller
Cc: socketcan, kuznet, urs.thuermann, yoshfuji, kaber, jmorris,
remi.denis-courmont, pekkas, sri, vladislav.yasevich, tj,
eric.dumazet, lizf, joe, shemminger, hadi, ebiederm, adobriyan,
jpirko, johannes.berg, daniel.lezcano, xemul, socketcan-core,
netdev, linux-sctp, torvalds
In-Reply-To: <20101111.182939.258124014.davem@davemloft.net>
> I want whatever you replace it with to be equivalent for
> object tracking purposes.
In nearly all of the cases I fixed, the socket inode is already
provided, which serves as a perfectly good unique identifier. Would you
prefer I include that information twice?
-Dan
^ permalink raw reply
* Re: can-bcm: fix minor heap overflow
From: Simon Horman @ 2010-11-12 2:39 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: David Miller, Linux Netdev List, Dan Rosenberg, Linus Torvalds,
Urs Thuermann, security
In-Reply-To: <4CDB1856.4040001@hartkopp.net>
On Wed, Nov 10, 2010 at 11:10:30PM +0100, Oliver Hartkopp wrote:
> On 64-bit platforms the ASCII representation of a pointer may be up to 17
> bytes long. This patch increases the length of the buffer accordingly.
>
> http://marc.info/?l=linux-netdev&m=128872251418192&w=2
>
> Reported-by: Dan Rosenberg <drosenberg@vsecurity.com>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> CC: Linus Torvalds <torvalds@linux-foundation.org>
>
> ---
>
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 08ffe9e..6faa825 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -125,7 +125,7 @@ struct bcm_sock {
> struct list_head tx_ops;
> unsigned long dropped_usr_msgs;
> struct proc_dir_entry *bcm_proc_read;
> - char procname [9]; /* pointer printed in ASCII with \0 */
> + char procname [20]; /* pointer printed in ASCII with \0 */
> };
If the string may be up to 17 bytes long why are you allocating 20?
> static inline struct bcm_sock *bcm_sk(const struct sock *sk)
^ permalink raw reply
* Re: can-bcm: fix minor heap overflow
From: Dan Rosenberg @ 2010-11-12 2:43 UTC (permalink / raw)
To: Simon Horman
Cc: Oliver Hartkopp, David Miller, Linux Netdev List, Linus Torvalds,
Urs Thuermann, security
In-Reply-To: <20101112023950.GA8145@verge.net.au>
>
> If the string may be up to 17 bytes long why are you allocating 20?
>
In Oliver's defense, this doesn't matter even a little bit. The
structure will be allocated with kmalloc-1024 either way.
-Dan
^ permalink raw reply
* Re: can-bcm: fix minor heap overflow
From: Simon Horman @ 2010-11-12 2:48 UTC (permalink / raw)
To: Dan Rosenberg
Cc: Oliver Hartkopp, David Miller, Linux Netdev List, Linus Torvalds,
Urs Thuermann, security
In-Reply-To: <1289529838.3090.209.camel@Dan>
On Thu, Nov 11, 2010 at 09:43:58PM -0500, Dan Rosenberg wrote:
>
> >
> > If the string may be up to 17 bytes long why are you allocating 20?
> >
>
> In Oliver's defense, this doesn't matter even a little bit. The
> structure will be allocated with kmalloc-1024 either way.
I agree that its very unlikely to make any difference.
I am just curious.
^ permalink raw reply
* Re: [PATCH 4/10] Fix leaking of kernel heap addresses in net/
From: David Miller @ 2010-11-12 2:49 UTC (permalink / raw)
To: drosenberg-PiUznwcHFHrqlBn2x/YWAg
Cc: urs.thuermann-l29pVbxQd1IUtdQbppsyvg,
socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
linux-sctp-u79uwXL29TY76Z2rM5mHXA,
shemminger-ZtmgI6mnKB3QT0dZR+AlfA, xemul-GEFAQzZX7r8dnm+yROfE0A,
pekkas-UjJjq++bwZ7HOG6cAo2yLw,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
jmorris-gx6/JNMH7DfYtjvyW6yDsg, kuznet-v/Mj1YrvjDBInbfyfbPRSQ,
adobriyan-Re5JQEeQqe8AvxtiuMwx3w, sri-r/Jw6+rmf7HQT0dZR+AlfA,
johannes.berg-ral2JQCrhuEAvxtiuMwx3w, hadi-jkUAjuhPggJWk0Htik3J/w,
vladislav.yasevich-VXdhtT5mjnY, lizf-BthXqXjhjHXQFUHtdCDX3A,
tj-DgEjT+Ai2ygdnm+yROfE0A,
remi.denis-courmont-xNZwKgViW5gAvxtiuMwx3w,
daniel.lezcano-GANU6spQydw, jpirko-H+wXaHxf7aLQT0dZR+AlfA,
yoshfuji-VfPWfsRibaP+Ru+s062T9g, socketcan-fJ+pQTUTwRTk1uMJSBkQmQ,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, netdev-u79uwXL29TY76Z2rM5mHXA,
joe-6d6DIl74uiNBDgjK7y7TUQ,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
kaber-dcUjhNyLwpNeoWH0uzbU5w
In-Reply-To: <1289529269.3090.207.camel@Dan>
From: Dan Rosenberg <drosenberg-PiUznwcHFHrqlBn2x/YWAg@public.gmane.org>
Date: Thu, 11 Nov 2010 21:34:29 -0500
>
>> I want whatever you replace it with to be equivalent for
>> object tracking purposes.
>
> In nearly all of the cases I fixed, the socket inode is already
> provided, which serves as a perfectly good unique identifier. Would you
> prefer I include that information twice?
The problem is that the socket inode is not available in a certain
subclass of cases, so the transformation is not equivalent.
Why not attack this at the heart of where your concern is, and hack
the %p format handling to do whatever it is you like instead of
patching code all over the tree?
^ permalink raw reply
* Re: [PATCH 4/10] Fix leaking of kernel heap addresses in net/
From: Dan Rosenberg @ 2010-11-12 2:51 UTC (permalink / raw)
To: David Miller
Cc: socketcan, kuznet, urs.thuermann, yoshfuji, kaber, jmorris,
remi.denis-courmont, pekkas, sri, vladislav.yasevich, tj,
eric.dumazet, lizf, joe, shemminger, hadi, ebiederm, adobriyan,
jpirko, johannes.berg, daniel.lezcano, xemul, socketcan-core,
netdev, linux-sctp, torvalds
In-Reply-To: <20101111.184902.233699247.davem@davemloft.net>
> >
> >> I want whatever you replace it with to be equivalent for
> >> object tracking purposes.
> >
> > In nearly all of the cases I fixed, the socket inode is already
> > provided, which serves as a perfectly good unique identifier. Would you
> > prefer I include that information twice?
>
> The problem is that the socket inode is not available in a certain
> subclass of cases, so the transformation is not equivalent.
>
> Why not attack this at the heart of where your concern is, and hack
> the %p format handling to do whatever it is you like instead of
> patching code all over the tree?
This has already been suggested, and I agree it is a much better
approach. If I take this approach, and find some suitable substitute
for those cases where the socket inode is not available, will you
consider these changes?
-Dan
^ permalink raw reply
* Re: [PATCH 4/10] Fix leaking of kernel heap addresses in net/
From: David Miller @ 2010-11-12 2:59 UTC (permalink / raw)
To: drosenberg-PiUznwcHFHrqlBn2x/YWAg
Cc: urs.thuermann-l29pVbxQd1IUtdQbppsyvg,
socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
linux-sctp-u79uwXL29TY76Z2rM5mHXA,
shemminger-ZtmgI6mnKB3QT0dZR+AlfA, xemul-GEFAQzZX7r8dnm+yROfE0A,
pekkas-UjJjq++bwZ7HOG6cAo2yLw,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
jmorris-gx6/JNMH7DfYtjvyW6yDsg, kuznet-v/Mj1YrvjDBInbfyfbPRSQ,
adobriyan-Re5JQEeQqe8AvxtiuMwx3w, sri-r/Jw6+rmf7HQT0dZR+AlfA,
johannes.berg-ral2JQCrhuEAvxtiuMwx3w, hadi-jkUAjuhPggJWk0Htik3J/w,
vladislav.yasevich-VXdhtT5mjnY, lizf-BthXqXjhjHXQFUHtdCDX3A,
tj-DgEjT+Ai2ygdnm+yROfE0A,
remi.denis-courmont-xNZwKgViW5gAvxtiuMwx3w,
daniel.lezcano-GANU6spQydw, jpirko-H+wXaHxf7aLQT0dZR+AlfA,
yoshfuji-VfPWfsRibaP+Ru+s062T9g, socketcan-fJ+pQTUTwRTk1uMJSBkQmQ,
ebiederm-aS9lmoZGLiVWk0Htik3J/w, netdev-u79uwXL29TY76Z2rM5mHXA,
joe-6d6DIl74uiNBDgjK7y7TUQ,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
kaber-dcUjhNyLwpNeoWH0uzbU5w
In-Reply-To: <1289530264.3090.212.camel@Dan>
From: Dan Rosenberg <drosenberg-PiUznwcHFHrqlBn2x/YWAg@public.gmane.org>
Date: Thu, 11 Nov 2010 21:51:04 -0500
> This has already been suggested, and I agree it is a much better
> approach. If I take this approach, and find some suitable substitute
> for those cases where the socket inode is not available, will you
> consider these changes?
I will consider an approach where the keys reported allow object
tracking equally as the actual pointers allow for right now.
I've said that this is my criteria about 3 times now.
^ permalink raw reply
* Re: Kernel rwlock design, Multicore and IGMP
From: Cypher Wu @ 2010-11-12 3:32 UTC (permalink / raw)
To: Eric Dumazet; +Cc: linux-kernel, netdev
In-Reply-To: <1289489007.17691.1310.camel@edumazet-laptop>
On Thu, Nov 11, 2010 at 11:23 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le jeudi 11 novembre 2010 à 21:49 +0800, Cypher Wu a écrit :
>
> Hi
>
> CC netdev, since you ask questions about network stuff _and_ rwlock
>
>
>> I'm using TILEPro and its rwlock in kernel is a liitle different than
>> other platforms. It have a priority for write lock that when tried it
>> will block the following read lock even if read lock is hold by
>> others. Its code can be read in Linux Kernel 2.6.36 in
>> arch/tile/lib/spinlock_32.c.
>
> This seems a bug to me.
>
> read_lock() can be nested. We used such a schem in the past in iptables
> (it can re-enter itself),
> and we used instead a spinlock(), but with many discussions with lkml
> and Linus himself if I remember well.
>
It seems not a problem that read_lock() can be nested or not since
rwlock doesn't have 'owner', it's just that should we give
write_lock() a priority than read_lock() since if there have a lot
read_lock()s then they'll starve write_lock().
We should work out a well defined behavior so all the
platform-dependent raw_rwlock has to design under that principle.
>
>>
>> That different could cause a deadlock in kernel if we join/leave
>> Multicast Group simultaneous and frequently on mutlicores. IGMP
>> message is sent by
>>
>> igmp_ifc_timer_expire() -> igmpv3_send_cr() -> igmpv3_sendpack()
>>
>> in timer interrupt, igmpv3_send_cr() will generate the sk_buff for
>> IGMP message with mc_list_lock read locked and then call
>> igmpv3_sendpack() with it unlocked.
>> But if we have so many join/leave messages have to generate and it
>> can't be sent in one sk_buff then igmpv3_send_cr() -> add_grec() will
>> call igmpv3_sendpack() to send it and reallocate a new buffer. When
>> the message is sent:
>>
>> __mkroute_output() -> ip_check_mc()
>>
>> will read lock mc_list_lock again. If there is another core is try
>> write lock mc_list_lock between the two read lock, then deadlock
>> ocurred.
>>
>> The rwlock on other platforms I've check, say, PowerPC, x86, ARM, is
>> just read lock shared and write_lock mutex, so if we've hold read lock
>> the write lock will just wait, and if there have a read lock again it
>> will success.
>>
>> So, What's the criteria of rwlock design in the Linux kernel? Is that
>> read lock re-hold of IGMP a design error in Linux kernel, or the read
>> lock has to be design like that?
>>
>
> Well, we try to get rid of all rwlocks in performance critical sections.
>
> I would say, if you believe one rwlock can justify the special TILE
> behavior you tried to make, then we should instead migrate this rwlock
> to a RCU + spinlock schem (so that all arches benefit from this work,
> not only TILE)
IGMP in not very performance critical so maybe rwlock is enough?
>
>> There is a other thing, that the timer interrupt will start timer on
>> the same in_dev, should that be optimized?
>>
>
> Not sure I understand what you mean.
Since mc_list & mc_tomb is shared list in the kernel we needn't to
start multiple timers on different cores for them, we can synchronize
it on one core.
>
>> BTW: If we have so many cores, say 64, is there other things we have
>> to think about spinlock? If there have collisions ocurred, should we
>> just read the shared memory again and again, or just a very little
>> 'delay' is better? I've seen relax() is called in the implementation
>> of spinlock on TILEPro platform.
>> --
>
> Is TILE using ticket spinlocks ?
>
What ticket spinlocks means? Could you explain a little, pls:) I'm not
familiar with Linux kernel, I'm trying to get more understanding of it
since I'm working on Linux platform now.
>
>
^ permalink raw reply
* Re: Kernel rwlock design, Multicore and IGMP
From: Américo Wang @ 2010-11-12 6:28 UTC (permalink / raw)
To: Cypher Wu; +Cc: Eric Dumazet, linux-kernel, netdev
In-Reply-To: <AANLkTik93QUQxSDmwd4Qj-gXQiWWPzd68JPAYAHBAsHR@mail.gmail.com>
On Fri, Nov 12, 2010 at 11:32:59AM +0800, Cypher Wu wrote:
>>
>> Is TILE using ticket spinlocks ?
>>
Eric, yes.
>
>What ticket spinlocks means? Could you explain a little, pls:) I'm not
>familiar with Linux kernel, I'm trying to get more understanding of it
>since I'm working on Linux platform now.
>
You might want to search "ticket spinlock" with google. :)
Briefly speaking, it is introduced to solve unfairness
of the old spinlock implementation. In linux kernel, not all
arch implement this. X86 implementation is done by Nick with
asm code, while tile uses C to implement it.
^ permalink raw reply
* Re: Kernel rwlock design, Multicore and IGMP
From: Américo Wang @ 2010-11-12 7:13 UTC (permalink / raw)
To: Cypher Wu; +Cc: Eric Dumazet, linux-kernel, netdev
In-Reply-To: <AANLkTik93QUQxSDmwd4Qj-gXQiWWPzd68JPAYAHBAsHR@mail.gmail.com>
On Fri, Nov 12, 2010 at 11:32:59AM +0800, Cypher Wu wrote:
>On Thu, Nov 11, 2010 at 11:23 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> Le jeudi 11 novembre 2010 à 21:49 +0800, Cypher Wu a écrit :
>>
>> Hi
>>
>> CC netdev, since you ask questions about network stuff _and_ rwlock
>>
>>
>>> I'm using TILEPro and its rwlock in kernel is a liitle different than
>>> other platforms. It have a priority for write lock that when tried it
>>> will block the following read lock even if read lock is hold by
>>> others. Its code can be read in Linux Kernel 2.6.36 in
>>> arch/tile/lib/spinlock_32.c.
>>
>> This seems a bug to me.
>>
>> read_lock() can be nested. We used such a schem in the past in iptables
>> (it can re-enter itself),
>> and we used instead a spinlock(), but with many discussions with lkml
>> and Linus himself if I remember well.
>>
>It seems not a problem that read_lock() can be nested or not since
>rwlock doesn't have 'owner', it's just that should we give
>write_lock() a priority than read_lock() since if there have a lot
>read_lock()s then they'll starve write_lock().
>We should work out a well defined behavior so all the
>platform-dependent raw_rwlock has to design under that principle.
It is a known weakness of rwlock, it is designed like that. :)
The solution is to use RCU or seqlock, but I don't think seqlock
is proper for this case you described. So, try RCU lock.
^ permalink raw reply
* Re: [PATCH 4/10] Fix leaking of kernel heap addresses in net/
From: Eric Dumazet @ 2010-11-12 7:23 UTC (permalink / raw)
To: Dan Rosenberg
Cc: David Miller, socketcan, kuznet, urs.thuermann, yoshfuji, kaber,
jmorris, remi.denis-courmont, pekkas, sri, vladislav.yasevich, tj,
lizf, joe, shemminger, hadi, ebiederm, adobriyan, jpirko,
johannes.berg, daniel.lezcano, xemul, socketcan-core, netdev,
linux-sctp, torvalds
In-Reply-To: <1289529269.3090.207.camel@Dan>
Le jeudi 11 novembre 2010 à 21:34 -0500, Dan Rosenberg a écrit :
> > I want whatever you replace it with to be equivalent for
> > object tracking purposes.
>
> In nearly all of the cases I fixed, the socket inode is already
> provided, which serves as a perfectly good unique identifier. Would you
> prefer I include that information twice?
Oh well. Please read this answer carefuly.
Some facts to feed your next patch. I am very pleased you changed your
mind and that we keep useful information in kernel log.
1) Inode numbers are not guaranteed to be unique. Its a 32bit seq
number, and we dont check another socket inode use the same inode number
(after 2^32 allocations it can happens)
2) /proc/net/ files can deliver same "line" of information several
times, because of their implementation.
3) Because of SLAB_DESTROY_BY_RCU, same 'kernel socket pointer' can be
seen several times in /proc/net/tcp & /proc/net/udp, but really on
different "sockets"
4) Some good applications use both the socket pointer and inode number
(tuple) to filter out the [2] problem. Dont break them, please ?
Anything that might break an application must be at the very least
tunable.
In my opinion, a good thing would be :
- Use a special printf() format , aka "secure pointer", as Thomas
suggested.
- Make sure you print different opaque values for two different kernel
pointers. This is mandatory.
- Make sure the NULL pointer stay as a NULL pointer to not let the
hostile user know your secret, and to ease debugging stuff.
- Have security experts advice to chose a nice crypto function, maybe
jenkin hash. Not too slow would be nice.
static unsigned long securize_kpointers_rnd;
At boot time, stick a random value in this variable.
(Maybe make sure the 5 low order bits are 0)
unsigned long opacify_kptr(unsigned long ptr)
{
if (ptr == 0)
return ptr;
if (capable(CAP_NET_ADMIN))
return ptr;
return some_crypto_hash(ptr, &securize_kpointers_rnd);
}
At least, use a central point, so that we can easily add/change the
logic if needed.
Please provide this patch in kernel/printk.c for initial review, then
once everybody is OK, you can send one patch for net tree.
No need to send 10 patches if we dont agree on the general principle.
^ permalink raw reply
* Re: Kernel rwlock design, Multicore and IGMP
From: Eric Dumazet @ 2010-11-12 7:27 UTC (permalink / raw)
To: Américo Wang; +Cc: Cypher Wu, linux-kernel, netdev
In-Reply-To: <20101112071323.GB5660@cr0.nay.redhat.com>
Le vendredi 12 novembre 2010 à 15:13 +0800, Américo Wang a écrit :
> On Fri, Nov 12, 2010 at 11:32:59AM +0800, Cypher Wu wrote:
> >On Thu, Nov 11, 2010 at 11:23 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >> Le jeudi 11 novembre 2010 à 21:49 +0800, Cypher Wu a écrit :
> >>
> >> Hi
> >>
> >> CC netdev, since you ask questions about network stuff _and_ rwlock
> >>
> >>
> >>> I'm using TILEPro and its rwlock in kernel is a liitle different than
> >>> other platforms. It have a priority for write lock that when tried it
> >>> will block the following read lock even if read lock is hold by
> >>> others. Its code can be read in Linux Kernel 2.6.36 in
> >>> arch/tile/lib/spinlock_32.c.
> >>
> >> This seems a bug to me.
> >>
> >> read_lock() can be nested. We used such a schem in the past in iptables
> >> (it can re-enter itself),
> >> and we used instead a spinlock(), but with many discussions with lkml
> >> and Linus himself if I remember well.
> >>
> >It seems not a problem that read_lock() can be nested or not since
> >rwlock doesn't have 'owner', it's just that should we give
> >write_lock() a priority than read_lock() since if there have a lot
> >read_lock()s then they'll starve write_lock().
> >We should work out a well defined behavior so all the
> >platform-dependent raw_rwlock has to design under that principle.
>
AFAIK, Lockdep allows read_lock() to be nested.
> It is a known weakness of rwlock, it is designed like that. :)
>
Agreed.
> The solution is to use RCU or seqlock, but I don't think seqlock
> is proper for this case you described. So, try RCU lock.
In the IGMP case, it should be easy for the task owning a read_lock() to
pass a parameter to the called function saying 'I already own the
read_lock(), dont try to re-acquire it'
A RCU conversion is far more complex.
^ permalink raw reply
* Re: [RFC PATCH] network: return errors if we know tcp_connect failed
From: Patrick McHardy @ 2010-11-12 7:36 UTC (permalink / raw)
To: Hua Zhong
Cc: 'Eric Paris', netdev, linux-kernel, davem, kuznet, pekkas,
jmorris, yoshfuji
In-Reply-To: <00c201cb81eb$84e18160$8ea48420$@com>
On 11.11.2010 22:58, Hua Zhong wrote:
>> Yes, I realize this is little different than if the
>> SYN was dropped in the first network device, but it is different
>> because we know what happened! We know that connect() call failed
>> and that there isn't anything coming back.
>
> I would argue that -j DROP should behave exactly as the packet is dropped in the network, while -j REJECT should signal the failure to the application as soon as possible (which it doesn't seem to do).
It sends an ICMP error or TCP reset. Interpretation is up to TCP.
^ permalink raw reply
* Re: [PATCH] rtnetlink: Fix message size calculation for link messages
From: Patrick McHardy @ 2010-11-12 7:43 UTC (permalink / raw)
To: davem, netdev
In-Reply-To: <20101112014759.GA8491@canuck.infradead.org>
On 12.11.2010 02:47, Thomas Graf wrote:
> nlmsg_total_size() calculates the length of a netlink message
> including header and alignment. nla_total_size() calculates the
> space an individual attribute consumes which was meant to be used
> in this context.
>
> Also, ensure to account for the attribute header for the
> IFLA_INFO_XSTATS attribute as implementations of get_xstats_size()
> seem to assume that we do so.
>
> The addition of two message headers minus the missing attribute
> header resulted in a calculated message size that was larger than
> required. Therefore we never risked running out of skb tailroom.
>
> Signed-off-by: Thomas Graf <tgraf@infradead.org>
> Cc: Patrick McHardy <kaber@trash.net>
Looks good to me, thanks Thomas.
Acked-by: Patrick McHardy <kaber@trash.net>
^ permalink raw reply
* Re: [PATCH] netfilter: ipv6: fix overlap check for fragments
From: Shan Wei @ 2010-11-12 7:47 UTC (permalink / raw)
To: nicolas.dichtel, netdev@vger.kernel.org, netfilter-devel,
Patrick McHardy
Cc: David Miller
In-Reply-To: <4CD3F0F8.5030205@cn.fujitsu.com>
Hi Patrick:
Would you apply this to your tree. The similar patch for ipv6 has been
applied by David .
See http://git.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commit;h=f46421416fb6b91513fb687d6503142cd99034a5
Shan Wei wrote, at 11/05/2010 07:56 PM:
> The type of FRAG6_CB(prev)->offset is int, skb->len is *unsigned* int,
> and offset is int.
>
> Without this patch, type conversion occurred to this expression, when
> (FRAG6_CB(prev)->offset + prev->len) is less than offset.
>
>
> Signed-off-by: Shan Wei <shanwei@cn.fujitsu.com>
> ---
> net/ipv6/netfilter/nf_conntrack_reasm.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
> index 3a3f129..79d43aa 100644
> --- a/net/ipv6/netfilter/nf_conntrack_reasm.c
> +++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
> @@ -286,7 +286,7 @@ found:
>
> /* Check for overlap with preceding fragment. */
> if (prev &&
> - (NFCT_FRAG6_CB(prev)->offset + prev->len) - offset > 0)
> + (NFCT_FRAG6_CB(prev)->offset + prev->len) > offset)
> goto discard_fq;
>
> /* Look for overlap with succeeding segment. */
--
Best Regards
-----
Shan Wei
^ permalink raw reply
* Re: [PATCH] netfilter: ipv6: fix overlap check for fragments
From: Patrick McHardy @ 2010-11-12 7:52 UTC (permalink / raw)
To: Shan Wei
Cc: nicolas.dichtel, David Miller, netdev@vger.kernel.org,
netfilter-devel
In-Reply-To: <4CD3F0F8.5030205@cn.fujitsu.com>
On 05.11.2010 12:56, Shan Wei wrote:
> The type of FRAG6_CB(prev)->offset is int, skb->len is *unsigned* int,
> and offset is int.
>
> Without this patch, type conversion occurred to this expression, when
> (FRAG6_CB(prev)->offset + prev->len) is less than offset.
Applied, thanks Shan.
^ permalink raw reply
* Re: Kernel rwlock design, Multicore and IGMP
From: Américo Wang @ 2010-11-12 8:19 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Américo Wang, Cypher Wu, linux-kernel, netdev
In-Reply-To: <1289546874.17691.1774.camel@edumazet-laptop>
On Fri, Nov 12, 2010 at 08:27:54AM +0100, Eric Dumazet wrote:
>Le vendredi 12 novembre 2010 à 15:13 +0800, Américo Wang a écrit :
>> On Fri, Nov 12, 2010 at 11:32:59AM +0800, Cypher Wu wrote:
>> >On Thu, Nov 11, 2010 at 11:23 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> >> Le jeudi 11 novembre 2010 à 21:49 +0800, Cypher Wu a écrit :
>> >>
>> >> Hi
>> >>
>> >> CC netdev, since you ask questions about network stuff _and_ rwlock
>> >>
>> >>
>> >>> I'm using TILEPro and its rwlock in kernel is a liitle different than
>> >>> other platforms. It have a priority for write lock that when tried it
>> >>> will block the following read lock even if read lock is hold by
>> >>> others. Its code can be read in Linux Kernel 2.6.36 in
>> >>> arch/tile/lib/spinlock_32.c.
>> >>
>> >> This seems a bug to me.
>> >>
>> >> read_lock() can be nested. We used such a schem in the past in iptables
>> >> (it can re-enter itself),
>> >> and we used instead a spinlock(), but with many discussions with lkml
>> >> and Linus himself if I remember well.
>> >>
>> >It seems not a problem that read_lock() can be nested or not since
>> >rwlock doesn't have 'owner', it's just that should we give
>> >write_lock() a priority than read_lock() since if there have a lot
>> >read_lock()s then they'll starve write_lock().
>> >We should work out a well defined behavior so all the
>> >platform-dependent raw_rwlock has to design under that principle.
>>
>
>AFAIK, Lockdep allows read_lock() to be nested.
>
>> It is a known weakness of rwlock, it is designed like that. :)
>>
>
>Agreed.
>
Just for record, both Tile and X86 implement rwlock with a write-bias,
this somewhat reduces the write-starvation problem.
>> The solution is to use RCU or seqlock, but I don't think seqlock
>> is proper for this case you described. So, try RCU lock.
>
>In the IGMP case, it should be easy for the task owning a read_lock() to
>pass a parameter to the called function saying 'I already own the
>read_lock(), dont try to re-acquire it'
>
>A RCU conversion is far more complex.
>
Yup.
^ permalink raw reply
* Re: [PATCH net-next-2.6 v2] macvlan: lockless tx path
From: Patrick McHardy @ 2010-11-12 8:20 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Ben Greear, Ben Hutchings
In-Reply-To: <1289459644.17691.1011.camel@edumazet-laptop>
On 11.11.2010 08:14, Eric Dumazet wrote:
> macvlan is a stacked device, like tunnels. We should use the lockless
> mechanism we are using in tunnels and loopback.
>
> This patch completely removes locking in TX path.
>
> tx stat counters are added into existing percpu stat structure, renamed
> from rx_stats to pcpu_stats.
>
> Note : this reverts commit 2c11455321f37 (macvlan: add multiqueue
> capability)
>
> Note : rx_errors converted to a 32bit counter, like tx_dropped, since
> they dont need 64bit range.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Patrick McHardy <kaber@trash.net>
> Cc: Ben Greear <greearb@candelatech.com>
> Cc: Ben Hutchings <bhutchings@solarflare.com>
> ---
> V2: correct kerneldoc
> u32 for tx_dropped and rx_errors
Looks good to me.
Acked-by: Patrick McHardy <kaber@trash.net>
^ permalink raw reply
* Re: Kernel rwlock design, Multicore and IGMP
From: Yong Zhang @ 2010-11-12 9:09 UTC (permalink / raw)
To: Américo Wang; +Cc: Eric Dumazet, Cypher Wu, linux-kernel, netdev
In-Reply-To: <20101112081945.GA5949@cr0.nay.redhat.com>
On Fri, Nov 12, 2010 at 4:19 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> On Fri, Nov 12, 2010 at 08:27:54AM +0100, Eric Dumazet wrote:
>>Le vendredi 12 novembre 2010 à 15:13 +0800, Américo Wang a écrit :
>>> On Fri, Nov 12, 2010 at 11:32:59AM +0800, Cypher Wu wrote:
>>> >On Thu, Nov 11, 2010 at 11:23 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>> >> Le jeudi 11 novembre 2010 à 21:49 +0800, Cypher Wu a écrit :
>>> >>
>>> >> Hi
>>> >>
>>> >> CC netdev, since you ask questions about network stuff _and_ rwlock
>>> >>
>>> >>
>>> >>> I'm using TILEPro and its rwlock in kernel is a liitle different than
>>> >>> other platforms. It have a priority for write lock that when tried it
>>> >>> will block the following read lock even if read lock is hold by
>>> >>> others. Its code can be read in Linux Kernel 2.6.36 in
>>> >>> arch/tile/lib/spinlock_32.c.
>>> >>
>>> >> This seems a bug to me.
>>> >>
>>> >> read_lock() can be nested. We used such a schem in the past in iptables
>>> >> (it can re-enter itself),
>>> >> and we used instead a spinlock(), but with many discussions with lkml
>>> >> and Linus himself if I remember well.
>>> >>
>>> >It seems not a problem that read_lock() can be nested or not since
>>> >rwlock doesn't have 'owner', it's just that should we give
>>> >write_lock() a priority than read_lock() since if there have a lot
>>> >read_lock()s then they'll starve write_lock().
>>> >We should work out a well defined behavior so all the
>>> >platform-dependent raw_rwlock has to design under that principle.
>>>
>>
>>AFAIK, Lockdep allows read_lock() to be nested.
>>
>>> It is a known weakness of rwlock, it is designed like that. :)
>>>
>>
>>Agreed.
>>
>
> Just for record, both Tile and X86 implement rwlock with a write-bias,
> this somewhat reduces the write-starvation problem.
Are you sure(on x86)?
It seems that we never realize writer-bias rwlock.
Thanks,
Yong
>
>
>>> The solution is to use RCU or seqlock, but I don't think seqlock
>>> is proper for this case you described. So, try RCU lock.
>>
>>In the IGMP case, it should be easy for the task owning a read_lock() to
>>pass a parameter to the called function saying 'I already own the
>>read_lock(), dont try to re-acquire it'
>>
>>A RCU conversion is far more complex.
>>
>
> Yup.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply
* Re: Kernel rwlock design, Multicore and IGMP
From: Américo Wang @ 2010-11-12 9:18 UTC (permalink / raw)
To: Yong Zhang
Cc: Américo Wang, Eric Dumazet, Cypher Wu, linux-kernel, netdev
In-Reply-To: <AANLkTik-KDvc2fgH91vBHGT6vqxbZrv=9DoQknujPWy2@mail.gmail.com>
On Fri, Nov 12, 2010 at 05:09:45PM +0800, Yong Zhang wrote:
>On Fri, Nov 12, 2010 at 4:19 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
>> On Fri, Nov 12, 2010 at 08:27:54AM +0100, Eric Dumazet wrote:
>>>Le vendredi 12 novembre 2010 à 15:13 +0800, Américo Wang a écrit :
>>>> On Fri, Nov 12, 2010 at 11:32:59AM +0800, Cypher Wu wrote:
>>>> >On Thu, Nov 11, 2010 at 11:23 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>>> >> Le jeudi 11 novembre 2010 à 21:49 +0800, Cypher Wu a écrit :
>>>> >>
>>>> >> Hi
>>>> >>
>>>> >> CC netdev, since you ask questions about network stuff _and_ rwlock
>>>> >>
>>>> >>
>>>> >>> I'm using TILEPro and its rwlock in kernel is a liitle different than
>>>> >>> other platforms. It have a priority for write lock that when tried it
>>>> >>> will block the following read lock even if read lock is hold by
>>>> >>> others. Its code can be read in Linux Kernel 2.6.36 in
>>>> >>> arch/tile/lib/spinlock_32.c.
>>>> >>
>>>> >> This seems a bug to me.
>>>> >>
>>>> >> read_lock() can be nested. We used such a schem in the past in iptables
>>>> >> (it can re-enter itself),
>>>> >> and we used instead a spinlock(), but with many discussions with lkml
>>>> >> and Linus himself if I remember well.
>>>> >>
>>>> >It seems not a problem that read_lock() can be nested or not since
>>>> >rwlock doesn't have 'owner', it's just that should we give
>>>> >write_lock() a priority than read_lock() since if there have a lot
>>>> >read_lock()s then they'll starve write_lock().
>>>> >We should work out a well defined behavior so all the
>>>> >platform-dependent raw_rwlock has to design under that principle.
>>>>
>>>
>>>AFAIK, Lockdep allows read_lock() to be nested.
>>>
>>>> It is a known weakness of rwlock, it is designed like that. :)
>>>>
>>>
>>>Agreed.
>>>
>>
>> Just for record, both Tile and X86 implement rwlock with a write-bias,
>> this somewhat reduces the write-starvation problem.
>
>Are you sure(on x86)?
>
>It seems that we never realize writer-bias rwlock.
>
Try
% grep RW_LOCK_BIAS -nr arch/x86
*And* read the code to see how it works. :)
Note, on Tile, it uses a little different algorithm.
^ permalink raw reply
* Re: Kernel rwlock design, Multicore and IGMP
From: Eric Dumazet @ 2010-11-12 9:22 UTC (permalink / raw)
To: Américo Wang; +Cc: Cypher Wu, linux-kernel, netdev
In-Reply-To: <20101112081945.GA5949@cr0.nay.redhat.com>
Le vendredi 12 novembre 2010 à 16:19 +0800, Américo Wang a écrit :
> On Fri, Nov 12, 2010 at 08:27:54AM +0100, Eric Dumazet wrote:
> >A RCU conversion is far more complex.
> >
>
> Yup.
Well, actually this is easy in this case.
I'll post a patch to do this RCU conversion.
^ permalink raw reply
* Re: Kernel rwlock design, Multicore and IGMP
From: Américo Wang @ 2010-11-12 9:33 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Américo Wang, Cypher Wu, linux-kernel, netdev
In-Reply-To: <1289553759.3185.1.camel@edumazet-laptop>
On Fri, Nov 12, 2010 at 10:22:39AM +0100, Eric Dumazet wrote:
>Le vendredi 12 novembre 2010 à 16:19 +0800, Américo Wang a écrit :
>> On Fri, Nov 12, 2010 at 08:27:54AM +0100, Eric Dumazet wrote:
>
>> >A RCU conversion is far more complex.
>> >
>>
>> Yup.
>
>
>Well, actually this is easy in this case.
>
>I'll post a patch to do this RCU conversion.
>
Cool! Please keep me in Cc.
Some conversions from rwlock to RCU are indeed complex,
don't know about this case.
Anyway, patch please. :)
^ permalink raw reply
* [PATCH] r8169: fix checksum broken
From: Shan Wei @ 2010-11-12 10:15 UTC (permalink / raw)
To: romieu; +Cc: netdev@vger.kernel.org, David Miller
If r8196 received packets with invalid sctp/igmp(not tcp, udp) checksum, r8196 set skb->ip_summed
wit CHECKSUM_UNNECESSARY. This cause that upper protocol don't check checksum field.
I am not family with r8196 driver. I try to guess the meaning of RxProtoIP and IPFail.
RxProtoIP stands for received IPv4 packet that upper protocol is not tcp and udp.
!(opts1 & IPFail) is true means that driver correctly to check checksum in IPv4 header.
If it's right, I think we should not set ip_summed wit CHECKSUM_UNNECESSARY for my sctp packets
with invalid checksum.
If it's not right, please tell me.
Signed-off-by: Shan Wei <shanwei@cn.fujitsu.com>
---
drivers/net/r8169.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index d88ce9f..2cf6c2e 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -4440,8 +4440,7 @@ static inline void rtl8169_rx_csum(struct sk_buff *skb, u32 opts1)
u32 status = opts1 & RxProtoMask;
if (((status == RxProtoTCP) && !(opts1 & TCPFail)) ||
- ((status == RxProtoUDP) && !(opts1 & UDPFail)) ||
- ((status == RxProtoIP) && !(opts1 & IPFail)))
+ ((status == RxProtoUDP) && !(opts1 & UDPFail)))
skb->ip_summed = CHECKSUM_UNNECESSARY;
else
skb_checksum_none_assert(skb);
--
1.6.3.3
^ permalink raw reply related
* Re: Kernel rwlock design, Multicore and IGMP
From: Cypher Wu @ 2010-11-12 11:06 UTC (permalink / raw)
To: Américo Wang; +Cc: Yong Zhang, Eric Dumazet, linux-kernel, netdev
In-Reply-To: <20101112091818.GB5949@cr0.nay.redhat.com>
2010/11/12 Américo Wang <xiyou.wangcong@gmail.com>:
> On Fri, Nov 12, 2010 at 05:09:45PM +0800, Yong Zhang wrote:
>>On Fri, Nov 12, 2010 at 4:19 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
>>> On Fri, Nov 12, 2010 at 08:27:54AM +0100, Eric Dumazet wrote:
>>>>Le vendredi 12 novembre 2010 à 15:13 +0800, Américo Wang a écrit :
>>>>> On Fri, Nov 12, 2010 at 11:32:59AM +0800, Cypher Wu wrote:
>>>>> >On Thu, Nov 11, 2010 at 11:23 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>>>> >> Le jeudi 11 novembre 2010 à 21:49 +0800, Cypher Wu a écrit :
>>>>> >>
>>>>> >> Hi
>>>>> >>
>>>>> >> CC netdev, since you ask questions about network stuff _and_ rwlock
>>>>> >>
>>>>> >>
>>>>> >>> I'm using TILEPro and its rwlock in kernel is a liitle different than
>>>>> >>> other platforms. It have a priority for write lock that when tried it
>>>>> >>> will block the following read lock even if read lock is hold by
>>>>> >>> others. Its code can be read in Linux Kernel 2.6.36 in
>>>>> >>> arch/tile/lib/spinlock_32.c.
>>>>> >>
>>>>> >> This seems a bug to me.
>>>>> >>
>>>>> >> read_lock() can be nested. We used such a schem in the past in iptables
>>>>> >> (it can re-enter itself),
>>>>> >> and we used instead a spinlock(), but with many discussions with lkml
>>>>> >> and Linus himself if I remember well.
>>>>> >>
>>>>> >It seems not a problem that read_lock() can be nested or not since
>>>>> >rwlock doesn't have 'owner', it's just that should we give
>>>>> >write_lock() a priority than read_lock() since if there have a lot
>>>>> >read_lock()s then they'll starve write_lock().
>>>>> >We should work out a well defined behavior so all the
>>>>> >platform-dependent raw_rwlock has to design under that principle.
>>>>>
>>>>
>>>>AFAIK, Lockdep allows read_lock() to be nested.
>>>>
>>>>> It is a known weakness of rwlock, it is designed like that. :)
>>>>>
>>>>
>>>>Agreed.
>>>>
>>>
>>> Just for record, both Tile and X86 implement rwlock with a write-bias,
>>> this somewhat reduces the write-starvation problem.
>>
>>Are you sure(on x86)?
>>
>>It seems that we never realize writer-bias rwlock.
>>
>
> Try
>
> % grep RW_LOCK_BIAS -nr arch/x86
>
> *And* read the code to see how it works. :)
>
> Note, on Tile, it uses a little different algorithm.
>
It seems that rwlock on x86 and tile have different behavior, x86 use
RW_LOCK_BIAS, when read_lock() it will test if the lock is 0, and if
so then the read_lock() have to 'spinning', otherwise it dec the lock;
when write_lock() tried it first check if lock is It seems that rwlock
on x86 and tile have different behavior, x86 use RW_LOCK_BIAS and if
so, set lock to 0 and continue, otherwise it will 'spinning'.
I'm not very familiar with x86 architecture, but the code seems like
working that way.
^ 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