* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Al Viro @ 2014-12-08 18:35 UTC (permalink / raw)
To: Linus Torvalds
Cc: Kirill A. Shutemov, Linux Kernel Mailing List, linux-fsdevel,
Network Development
In-Reply-To: <CA+55aFzKGsYKh1HALoeFGYQ46uMHww0WdLf5N-9duNTgMSSFxQ@mail.gmail.com>
On Mon, Dec 08, 2014 at 10:23:26AM -0800, Linus Torvalds wrote:
> Did this actually use to work? Or is it an issue of "the new iov_iter
> is so generic that something that used to just return an error now
> 'works' and triggers the problem"?
Looks like it failed with EINVAL. Which might very well be the sane
reaction - if we run into a vmalloc/module address, act as if we failed
to get that page and exit the loop.
> > What's the sane way to grab struct page * for a vmalloc'ed address?
>
> So "vmalloc_to_page()" should work.
>
> However, it's actually fundamentally racy unless you can guarantee
> that the vmalloc()'ed area in question is stable (so you had better
> have done that allocation yourself, and be in control of the freeing,
> rather than "we look up random vmalloc'ed addresses).
If vfree(buffer) races with kernel_read() into buffer, we are so badly
fucked that stability of pointers to pages is the least of our concerns...
> In general, it's really a horrible thing to use, and tends to be a big
> red sign that "somebody misdesigned this badly"
More like "nobody has thought of that case", at a guess, but then I hadn't
been involved in finit_module() design - I don't even remember the discussions
around it. That would be what, something circa 3.7?
^ permalink raw reply
* Re: [ovs-dev] OVS Kernel Datapath development
From: Pravin Shelar @ 2014-12-08 18:30 UTC (permalink / raw)
To: Thomas Graf; +Cc: netdev, dev@openvswitch.org
In-Reply-To: <20141208171506.GC2835@pox.localdomain>
On Mon, Dec 8, 2014 at 9:15 AM, Thomas Graf <tgraf@noironetworks.com> wrote:
> On 12/07/14 at 08:47pm, Pravin Shelar wrote:
>> Since the beginning OVS kernel datapath development is primarily done
>> on external OVS repo. Now we have mostly synced upstream and external
>> OVS. So we have decided to change this process. New process is as
>> follows.
>>
>> 1. OVS feature development that involves kernel datapath should be
>> done on net-next tree datapath.
>> 2. Such feature patch series should be posted on netdev and ovs-dev
>> mailing list.
>> 3. Once review is done for entire series, kernel and OVS userspace
>> patches will be merged in respective repo.
>> 4. After the merge developer is suppose to send patches for external
>> kernel datapath along with old kernel compatibility code. So that we
>> can keep external datapath insync.
>
> +1
>
> Just to be clear, by respective repo do you mean net-next/net or will
> you maintain a net-next branch on git.kernel.org and continue doing
> pull requests?
OVS patches will directly go to net-next/net tree. I am not planning
on maintaining any tree on git.kernel.org.
^ permalink raw reply
* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Linus Torvalds @ 2014-12-08 18:23 UTC (permalink / raw)
To: Al Viro
Cc: Kirill A. Shutemov, Linux Kernel Mailing List, linux-fsdevel,
Network Development
In-Reply-To: <20141208181401.GD22149@ZenIV.linux.org.uk>
On Mon, Dec 8, 2014 at 10:14 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> iov_iter_get_pages() in ITER_KVEC case, trying to avoid get_user_pages_fast()
> and getting it wrong. FWIW, the reproducer is finit_module(fd, ....)
> where fd has been opened with O_DIRECT. In that case we get kernel_read()
> on O_DIRECT and the buffer has just been vmalloc'ed.
Ugh. That's horrid. Do we need to even support O_DIRECT in that case?
In general, we should *not* do IO on vmalloc'ed areas, although at
least the non-O_DIRECT case where we just memcpy() it as if it came
from user space is much better.
Did this actually use to work? Or is it an issue of "the new iov_iter
is so generic that something that used to just return an error now
'works' and triggers the problem"?
> What's the sane way to grab struct page * for a vmalloc'ed address?
So "vmalloc_to_page()" should work.
However, it's actually fundamentally racy unless you can guarantee
that the vmalloc()'ed area in question is stable (so you had better
have done that allocation yourself, and be in control of the freeing,
rather than "we look up random vmalloc'ed addresses).
In general, it's really a horrible thing to use, and tends to be a big
red sign that "somebody misdesigned this badly"
Linus
^ permalink raw reply
* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Al Viro @ 2014-12-08 18:20 UTC (permalink / raw)
To: Linus Torvalds
Cc: Kirill A. Shutemov, Linux Kernel Mailing List, linux-fsdevel,
Network Development
In-Reply-To: <CA+55aFxwrH4vsyw2ix=HqDK9Z=fw98xtOL8=57prm8qgVmAFfA@mail.gmail.com>
On Mon, Dec 08, 2014 at 10:14:13AM -0800, Linus Torvalds wrote:
> For a vmalloc() address, you'd have to actually walk the page tables.
> Which is a f*cking horrible idea. Don't do it. We do have a
> "vmalloc_to_page()" that does it, but the basic issue is that you damn
> well shouldn't do IO on vmalloc'ed addresses. vmalloc'ed addresses
> only exist in the first place to give a linear *virtual* mapping, if
> you want physical pages you shouldn't have mixed it up with vmalloc in
> the first place!
>
> Where the hell does this crop up, and who does this insane thing
> anyway? It's wrong. How did it ever work before?
finit_module() with O_DIRECT descriptor. And I suspect that "not well"
is the answer - it used to call get_user_pages_fast() in that case.
I certainly had missed that insanity during the analysis - we don't do
a lot of O_DIRECT IO to/from kernel addresses of any sort... This
codepath allows it ;-/ Ability to trigger it is equivalent to ability
to run any code in kernel mode, so it's not an additional security hole,
but...
^ permalink raw reply
* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Linus Torvalds @ 2014-12-08 18:14 UTC (permalink / raw)
To: Al Viro
Cc: Kirill A. Shutemov, Linux Kernel Mailing List, linux-fsdevel,
Network Development
In-Reply-To: <20141208180824.GC22149@ZenIV.linux.org.uk>
On Mon, Dec 8, 2014 at 10:08 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> FWIW, virt_to_page() is probably not OK to call on an address in the
> middle of vmalloc'ed area, is it?
See my email that crossed yours. No it is not.
> Would
> for (end = addr + len; addr < end; addr += PAGE_SIZE) {
> if (is_vmalloc_addr(addr))
> ACCESS_ONCE(*(char *)addr);
> get_page(*pages++ = virt_to_page(addr));
> }
> be a safe replacement for the loop in the above?
No. That "ACCESS_ONCE()" does nothing. It reads a byte from 'addr' in
the vmalloc space, and might cause a page fault to make sure it's
mapped, but that is still a no-op.
You can't do "virt_to_page()" on anything but the normal 1:1 kernel
mappings (and only for non-highmem pages at that).
For a vmalloc() address, you'd have to actually walk the page tables.
Which is a f*cking horrible idea. Don't do it. We do have a
"vmalloc_to_page()" that does it, but the basic issue is that you damn
well shouldn't do IO on vmalloc'ed addresses. vmalloc'ed addresses
only exist in the first place to give a linear *virtual* mapping, if
you want physical pages you shouldn't have mixed it up with vmalloc in
the first place!
Where the hell does this crop up, and who does this insane thing
anyway? It's wrong. How did it ever work before?
Linus
^ permalink raw reply
* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Al Viro @ 2014-12-08 18:14 UTC (permalink / raw)
To: Linus Torvalds
Cc: Kirill A. Shutemov, Linux Kernel Mailing List, linux-fsdevel,
Network Development
In-Reply-To: <CA+55aFz6izpVGxkziynRXg7zbHzteQyPJH1Kg91ZO3fzqn3LaQ@mail.gmail.com>
On Mon, Dec 08, 2014 at 10:07:55AM -0800, Linus Torvalds wrote:
> Which is in the vmalloc address space. So somebody used a vmalloc'ed
> address and tried to convert it to a physical address in order to look
> up the page.
>
> Which is not a valid operation, and the BUG_ON() is definitely proper.
>
> Now *why* something tried to do a virt_to_page() on a vmalloc'ed
> address, that I leave to others.
iov_iter_get_pages() in ITER_KVEC case, trying to avoid get_user_pages_fast()
and getting it wrong. FWIW, the reproducer is finit_module(fd, ....)
where fd has been opened with O_DIRECT. In that case we get kernel_read()
on O_DIRECT and the buffer has just been vmalloc'ed.
What's the sane way to grab struct page * for a vmalloc'ed address?
^ permalink raw reply
* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Al Viro @ 2014-12-08 18:08 UTC (permalink / raw)
To: Kirill A. Shutemov; +Cc: Linus Torvalds, linux-kernel, linux-fsdevel, netdev
In-Reply-To: <20141208175805.GB22149@ZenIV.linux.org.uk>
On Mon, Dec 08, 2014 at 05:58:05PM +0000, Al Viro wrote:
> It looks like the second VIRTUAL_BUG_ON() in __phys_addr(), most likely
> from __pa(), from virt_to_page(), from
> unsigned long addr = (unsigned long)v.iov_base, end;
> size_t len = v.iov_len + (*start = addr & (PAGE_SIZE - 1));
>
> if (len > maxpages * PAGE_SIZE)
> len = maxpages * PAGE_SIZE;
> addr &= ~(PAGE_SIZE - 1);
> for (end = addr + len; addr < end; addr += PAGE_SIZE)
> get_page(*pages++ = virt_to_page(addr));
> return len - *start;
> in iov_iter_get_pages(). And that's ITER_KVEC case there... Further
> call chain looks like dio_refill_pages(), from dio_get_page(), from
> do_direct_io(), eventually from kernel_read() and finit_module(),
> Presumably called on O_DIRECT descriptor...
FWIW, virt_to_page() is probably not OK to call on an address in the
middle of vmalloc'ed area, is it? Would
for (end = addr + len; addr < end; addr += PAGE_SIZE) {
if (is_vmalloc_addr(addr))
ACCESS_ONCE(*(char *)addr);
get_page(*pages++ = virt_to_page(addr));
}
be a safe replacement for the loop in the above?
^ permalink raw reply
* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Linus Torvalds @ 2014-12-08 18:07 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: Al Viro, Linux Kernel Mailing List, linux-fsdevel,
Network Development
In-Reply-To: <20141208164650.GB29028@node.dhcp.inet.fi>
On Mon, Dec 8, 2014 at 8:46 AM, Kirill A. Shutemov <kirill@shutemov.name> wrote:
>
> I guess this crash is related to the patchset.
Sounds likely.
> [ 102.338270] kernel BUG at /home/kas/git/public/linux-next/arch/x86/mm/physaddr.c:26!
So that's
VIRTUAL_BUG_ON((x > y) || !phys_addr_valid(x));
and the code disassembles to:
0: 48 01 f8 add %rdi,%rax
3: 48 39 c2 cmp %rax,%rdx
6: 72 1b jb 0x23
8: 0f b6 0d 9d 7a ec 00 movzbl 0xec7a9d(%rip),%ecx # 0xec7aac
f: 48 89 c2 mov %rax,%rdx
12: 48 d3 ea shr %cl,%rdx
15: 48 85 d2 test %rdx,%rdx
18: 75 09 jne 0x23
1a: 5d pop %rbp
1b: c3 retq
1c: 0f 1f 80 00 00 00 00 nopl 0x0(%rax)
23:* 0f 0b ud2 <-- trapping instruction
with thre relevant registers being
> [ 102.340011] RAX: 00004100174b4000 RBX: ffff880049c73b08 RCX: 0000000000000028
> [ 102.340011] RDX: 0000000000000041 RSI: ffff88015dc980a8 RDI: ffffc900174b4000
so we've taken the second case (the %rcx value is
"boot_cpu_data.x86_phys_bits", which is that "movzbl", and the %rdx
value is the shifted value of %rax).
So %rax seems to contain 'x' at that point, which means that 'y' should be
x - (__START_KERNEL_map - PAGE_OFFSET)
which means that the _original_ address should be that plus
__START_KERNEL_map, ie just x + PAGE_OFFSET.
So it smells like the original virtual address was that
ffffc900174b4000 that we still find in %rdi.
Which is in the vmalloc address space. So somebody used a vmalloc'ed
address and tried to convert it to a physical address in order to look
up the page.
Which is not a valid operation, and the BUG_ON() is definitely proper.
Now *why* something tried to do a virt_to_page() on a vmalloc'ed
address, that I leave to others.
Linus
^ permalink raw reply
* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Al Viro @ 2014-12-08 17:58 UTC (permalink / raw)
To: Kirill A. Shutemov; +Cc: Linus Torvalds, linux-kernel, linux-fsdevel, netdev
In-Reply-To: <20141208164650.GB29028@node.dhcp.inet.fi>
On Mon, Dec 08, 2014 at 06:46:50PM +0200, Kirill A. Shutemov wrote:
> I guess this crash is related to the patchset.
Might be. Do you have a reproducer for it?
It looks like the second VIRTUAL_BUG_ON() in __phys_addr(), most likely
from __pa(), from virt_to_page(), from
unsigned long addr = (unsigned long)v.iov_base, end;
size_t len = v.iov_len + (*start = addr & (PAGE_SIZE - 1));
if (len > maxpages * PAGE_SIZE)
len = maxpages * PAGE_SIZE;
addr &= ~(PAGE_SIZE - 1);
for (end = addr + len; addr < end; addr += PAGE_SIZE)
get_page(*pages++ = virt_to_page(addr));
return len - *start;
in iov_iter_get_pages(). And that's ITER_KVEC case there... Further
call chain looks like dio_refill_pages(), from dio_get_page(), from
do_direct_io(), eventually from kernel_read() and finit_module(),
Presumably called on O_DIRECT descriptor...
I'll try to reproduce it here, but if you have any reliable reproducer, it
would be very welcome (and would make a useful addition to LTP and/or
xfstests).
^ permalink raw reply
* Re: [PATCH net-next 2/3] netlink: IFLA_PHYS_SWITCH_ID to IFLA_PHYS_PARENT_ID
From: Andy Gospodarek @ 2014-12-08 17:49 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, sfeldma
In-Reply-To: <20141208164143.GI1885@nanopsycho.brq.redhat.com>
On Mon, Dec 08, 2014 at 05:41:43PM +0100, Jiri Pirko wrote:
> Mon, Dec 08, 2014 at 04:37:47PM CET, gospo@cumulusnetworks.com wrote:
> >On Mon, Dec 08, 2014 at 04:17:14PM +0100, Jiri Pirko wrote:
> >> Fri, Dec 05, 2014 at 07:02:16PM CET, gospo@cumulusnetworks.com wrote:
> >> >There has been much discussion about proper nomenclature to use for this
> >> >and I would prefer parent rather than calling every forwarding element a
> >> >switch.
> >>
> >> Andy, I must say I really do not like just plain "parent". It is really
> >> not clear what it means as it can mean 1000 things.
> >>
> >> I know "switch" is not ideal but everytime anyone is talking about these
> >> kind of forwarding devices, they use word "switch" even if it is not
> >> accurate and everyone knows what they are talking about. Nobody uses
> >> "parent".
> >
> >Well of course they are not going to use it until it's committed. ;-)
>
>
> Do you seriously expect people talking about "parents" instead of
"Parent device" -- absolutely
> "switches". I doubt that...
Agree to disagree, I guess!
^ permalink raw reply
* Re: [ovs-discuss] kernel panic receiving flooded VXLAN traffic with OVS
From: Jesse Gross @ 2014-12-08 17:33 UTC (permalink / raw)
To: Nicholas Bastin; +Cc: Jay Vosburgh, netdev, discuss@openvswitch.org
In-Reply-To: <CADmMkWdGhw3Rn3gMEtAV8ESZB-k+uAK=Ed=LwsShKOtyHdJS_g@mail.gmail.com>
On Sat, Dec 6, 2014 at 2:47 PM, Nicholas Bastin <nick.bastin@gmail.com> wrote:
> On Fri, Dec 5, 2014 at 4:51 PM, Jesse Gross <jesse@nicira.com> wrote:
>>
>> I don't think there is anything inherently wrong with aggregating TCP
>> segments in VXLAN that are not destined for the local host. This is
>> conceptually the same as doing aggregation for TCP packets where we
>> only perform L2 bridging - in theory we shouldn't look at the upper
>> layers but it is fine as long as we faithfully reconstruct it on the
>> way out.
>
>
> But you don't faithfully reconstruct what the user originally sent - in-path
> reassembly is always wrong, which is why hardware switches don't do it (by
> default, anyhow). If you configure a middlebox to do some kind of
> assembly/translation/whatever work for you, that's fine, but something that
> advertises itself as a "switch" or "router" should definitely not do this by
> default.
>
> If you reassemble frames you completely obviate any kind of PMTU-D or
> configured MTU that your user is using, and this breaks a lot of paths. We
> completely disable all GRO/TSO/etc., but if you are able to determine that a
> packet is not destined for the local host you should definitely not mutate
> it.
If you look at the implementation of GRO/TSO, I think you will see
that it does in fact faithfully reconstruct the original message and
path MTU discovery is preserved. On Linux systems, GRO is enabled by
default for all workloads - including those that do not result in
local termination such as bridging.
^ permalink raw reply
* Re: [PATCH net] netlink: use jhash as hashfn for rhashtable
From: Hannes Frederic Sowa @ 2014-12-08 17:25 UTC (permalink / raw)
To: Dave Taht; +Cc: Daniel Borkmann, davem, netdev, Herbert Xu, Thomas Graf
In-Reply-To: <CAA93jw5bAqxbpxyOHEHd6XSEn4-HK0a9qw3Cb_Vecb8Nz=xbfg@mail.gmail.com>
On Mon, Dec 8, 2014, at 18:20, Dave Taht wrote:
> On Mon, Dec 8, 2014 at 8:56 AM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> > On Mo, 2014-12-08 at 17:30 +0100, Daniel Borkmann wrote:
> >> For netlink, we shouldn't be using arch_fast_hash() as a hashing
> >> discipline, but rather jhash() instead.
>
> I am not particularly happy with the amount of entropy in
>
> static inline u32 ipv6_addr_hash(const struct in6_addr *a)
> {
> #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG ==
> 64
> const unsigned long *ul = (const unsigned long *)a;
> unsigned long x = ul[0] ^ ul[1];
>
> return (u32)(x ^ (x >> 32));
> #else
> return (__force u32)(a->s6_addr32[0] ^ a->s6_addr32[1] ^
> a->s6_addr32[2] ^ a->s6_addr32[3]);
> #endif
> }
>
> is this worth improving somehow?
>
That's e.g. the reason why we have
commit 5a3da1fe9561828d0ca7eca664b16ec2b9bf0055
Author: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Fri Mar 15 11:32:30 2013 +0000
inet: limit length of fragment queue hash table bucket lists
Note, __ipv6_addr_jhash (xoring the upper 32 bit before jhashing them)
has the same problem. I currently cannot spot any problematic users in
the kernel, flow dissector hashes are insecure by nature, local
addresses normally don't have problems with hash collisions. But maybe I
should redo an audit. :)
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH net] netlink: use jhash as hashfn for rhashtable
From: Dave Taht @ 2014-12-08 17:20 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Daniel Borkmann, davem@davemloft.net, netdev@vger.kernel.org,
Herbert Xu, Thomas Graf
In-Reply-To: <1418057780.29477.12.camel@localhost>
On Mon, Dec 8, 2014 at 8:56 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On Mo, 2014-12-08 at 17:30 +0100, Daniel Borkmann wrote:
>> For netlink, we shouldn't be using arch_fast_hash() as a hashing
>> discipline, but rather jhash() instead.
I am not particularly happy with the amount of entropy in
static inline u32 ipv6_addr_hash(const struct in6_addr *a)
{
#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
const unsigned long *ul = (const unsigned long *)a;
unsigned long x = ul[0] ^ ul[1];
return (u32)(x ^ (x >> 32));
#else
return (__force u32)(a->s6_addr32[0] ^ a->s6_addr32[1] ^
a->s6_addr32[2] ^ a->s6_addr32[3]);
#endif
}
is this worth improving somehow?
>> Since netlink sockets can be opened by any user, a local attacker
>> would be able to easily create collisions with the DPDK-derived
>> arch_fast_hash(), which trades off performance for security by
>> using crc32 CPU instructions on x86_64.
>>
>> While it might have a legimite use case in other places, it should
>> be avoided in netlink context, though. As rhashtable's API is very
>> flexible, we could later on still decide on other hashing disciplines,
>> if legitimate.
>>
>> Reference: http://thread.gmane.org/gmane.linux.kernel/1844123
>> Fixes: e341694e3eb5 ("netlink: Convert netlink_lookup() to use RCU protected hash table")
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> Cc: Thomas Graf <tgraf@suug.ch>
>> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>> ---
>> net/netlink/af_netlink.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
>> index 0007b81..b6bf8e8 100644
>> --- a/net/netlink/af_netlink.c
>> +++ b/net/netlink/af_netlink.c
>> @@ -3130,7 +3130,7 @@ static int __init netlink_proto_init(void)
>> .head_offset = offsetof(struct netlink_sock, node),
>> .key_offset = offsetof(struct netlink_sock, portid),
>> .key_len = sizeof(u32), /* portid */
>> - .hashfn = arch_fast_hash,
>> + .hashfn = jhash,
>> .max_shift = 16, /* 64K */
>> .grow_decision = rht_grow_above_75,
>> .shrink_decision = rht_shrink_below_30,
>
> Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>
> In net-next, some time soon, we should try to let all function pointers
> to jhash() use one non-inline version. The other arch_fast_hash patch
> adds __jhash for x86-only, we can move it over to lib/.
>
> Thanks,
> Hannes
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Dave Täht
http://www.bufferbloat.net/projects/bloat/wiki/Upcoming_Talks
^ permalink raw reply
* Re: [ovs-dev] OVS Kernel Datapath development
From: Thomas Graf @ 2014-12-08 17:15 UTC (permalink / raw)
To: Pravin Shelar; +Cc: netdev, dev@openvswitch.org
In-Reply-To: <CALnjE+rn_Giv+8TM1_E2faBftRkivXMYHcjn-4Eq4Gu15r=CiQ@mail.gmail.com>
On 12/07/14 at 08:47pm, Pravin Shelar wrote:
> Since the beginning OVS kernel datapath development is primarily done
> on external OVS repo. Now we have mostly synced upstream and external
> OVS. So we have decided to change this process. New process is as
> follows.
>
> 1. OVS feature development that involves kernel datapath should be
> done on net-next tree datapath.
> 2. Such feature patch series should be posted on netdev and ovs-dev
> mailing list.
> 3. Once review is done for entire series, kernel and OVS userspace
> patches will be merged in respective repo.
> 4. After the merge developer is suppose to send patches for external
> kernel datapath along with old kernel compatibility code. So that we
> can keep external datapath insync.
+1
Just to be clear, by respective repo do you mean net-next/net or will
you maintain a net-next branch on git.kernel.org and continue doing
pull requests?
^ permalink raw reply
* Re: wl1251: NVS firmware data
From: Pali Rohár @ 2014-12-08 17:11 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Greg Kroah-Hartman, Ming Lei, Pavel Machek, John W. Linville,
Grazvydas Ignotas, linux-wireless@vger.kernel.org,
Network Development, Linux Kernel Mailing List, Ivaylo Dimitrov,
Aaro Koskinen, Kalle Valo, Sebastian Reichel, David Gnedt
In-Reply-To: <E5F3BB54-6365-4856-A231-A5FEDAEA217F@holtmann.org>
[-- Attachment #1: Type: Text/Plain, Size: 3623 bytes --]
On Monday 08 December 2014 18:05:37 Marcel Holtmann wrote:
> Hi Pali,
>
> >>>> On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
> >>>> /**
> >>>>
> >>>> + * request_firmware_prefer_user: - prefer usermode
> >>>> helper for loading firmware + * @firmware_p: pointer to
> >>>> firmware image
> >>>> + * @name: name of firmware file
> >>>> + * @device: device for which firmware is being loaded
> >>>> + *
> >>>> + * This function works pretty much like
> >>>> request_firmware(), but it prefer + * usermode helper. If
> >>>> usermode helper fails then it fallback to direct access.
> >>>> + * Usefull for dynamic or model specific firmware data.
> >>>> + **/
> >>>> +int request_firmware_prefer_user(const struct firmware
> >>>> **firmware_p, + const char
> >>>> *name, struct device *device) +{
> >>>> + int ret;
> >>>> + __module_get(THIS_MODULE);
> >>>> + ret = _request_firmware(firmware_p, name, device,
> >>>> + FW_OPT_UEVENT |
> >>>> FW_OPT_PREFER_USER); + module_put(THIS_MODULE);
> >>>> + return ret;
> >>>> +}
> >>>> +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
> >>>
> >>> I'd like to introduce request_firmware_user() which only
> >>> requests firmware from user space, and this way is simpler
> >>> and more flexible since we have request_firmware_direct()
> >>> already.
> >>
> >> Why would a driver care about what program provides the
> >> firmware? It shouldn't at all, and we want to get rid of
> >> the userspace firmware loader, not encourage drivers to
> >> use it "exclusively" at all.
> >
> > Do not remove it! Without userspace firmware loader it is
> > impossible to load dynamic firmware files.
>
> why is this dynamic in the first place. It does not sound like
> dynamic data to me at all. This is like the WiFi MAC
> address(es) or Bluetooth BD_ADDR. They are all static
> information. The only difference is that they are on the host
> accessibly filesystem or storage and not on the device
> itself.
>
> To be honest, for Bluetooth we solved this now. If the device
> is missing key information like the calibration data or
> BD_ADDR, then it comes up unconfigured. A userspace process
> can then go and load the right data into it and then the
> device becomes available as Bluetooth device.
>
> Trying to use request_firmware to load some random data and
> insist on going through userspace helper for that sounds
> crazy to me. Especially since we are trying hard to get away
> from the userspace loader. Forcing to keep it for new stuff
> sounds backwards to me.
>
> With the special Nokia partition in mind, why hasn't this been
> turned into a mountable filesystem or into a driver/subsystem
> that can access the data direct from the kernel. I advocated
> for this some time ago. Maybe there should be a special
> subsystem for access to these factory persistent information
> that drivers then just can access. I seem to remember that
> some systems provide these via ACPI. Why does the ARM
> platform has to be special here?
>
> And the problem of getting Ethernet and WiFi MAC address and
> Bluetooth BD_ADDR comes up many many times. Why not have
> something generic here. And don't tell me request_firmware is
> that generic solution ;)
>
> Regards
>
> Marcel
Hi Marcel. I think you did not understand this problem. This
discussion is not about mac address. Please read email thread
again and if there are some unclear pars, then ask. Thanks!
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: wl1251: NVS firmware data
From: Marcel Holtmann @ 2014-12-08 17:05 UTC (permalink / raw)
To: Pali Rohár
Cc: Greg Kroah-Hartman, Ming Lei, Pavel Machek, John W. Linville,
Grazvydas Ignotas, linux-wireless@vger.kernel.org,
Network Development, Linux Kernel Mailing List, Ivaylo Dimitrov,
Aaro Koskinen, Kalle Valo, Sebastian Reichel, David Gnedt
In-Reply-To: <201412081747.30965@pali>
Hi Pali,
>>>> On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
>>>> /**
>>>>
>>>> + * request_firmware_prefer_user: - prefer usermode helper
>>>> for loading firmware + * @firmware_p: pointer to firmware
>>>> image
>>>> + * @name: name of firmware file
>>>> + * @device: device for which firmware is being loaded
>>>> + *
>>>> + * This function works pretty much like
>>>> request_firmware(), but it prefer + * usermode helper. If
>>>> usermode helper fails then it fallback to direct access.
>>>> + * Usefull for dynamic or model specific firmware data.
>>>> + **/
>>>> +int request_firmware_prefer_user(const struct firmware
>>>> **firmware_p, + const char
>>>> *name, struct device *device) +{
>>>> + int ret;
>>>> + __module_get(THIS_MODULE);
>>>> + ret = _request_firmware(firmware_p, name, device,
>>>> + FW_OPT_UEVENT |
>>>> FW_OPT_PREFER_USER); + module_put(THIS_MODULE);
>>>> + return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
>>>
>>> I'd like to introduce request_firmware_user() which only
>>> requests firmware from user space, and this way is simpler
>>> and more flexible since we have request_firmware_direct()
>>> already.
>>
>> Why would a driver care about what program provides the
>> firmware? It shouldn't at all, and we want to get rid of the
>> userspace firmware loader, not encourage drivers to use it
>> "exclusively" at all.
>>
>
> Do not remove it! Without userspace firmware loader it is
> impossible to load dynamic firmware files.
why is this dynamic in the first place. It does not sound like dynamic data to me at all. This is like the WiFi MAC address(es) or Bluetooth BD_ADDR. They are all static information. The only difference is that they are on the host accessibly filesystem or storage and not on the device itself.
To be honest, for Bluetooth we solved this now. If the device is missing key information like the calibration data or BD_ADDR, then it comes up unconfigured. A userspace process can then go and load the right data into it and then the device becomes available as Bluetooth device.
Trying to use request_firmware to load some random data and insist on going through userspace helper for that sounds crazy to me. Especially since we are trying hard to get away from the userspace loader. Forcing to keep it for new stuff sounds backwards to me.
With the special Nokia partition in mind, why hasn't this been turned into a mountable filesystem or into a driver/subsystem that can access the data direct from the kernel. I advocated for this some time ago. Maybe there should be a special subsystem for access to these factory persistent information that drivers then just can access. I seem to remember that some systems provide these via ACPI. Why does the ARM platform has to be special here?
And the problem of getting Ethernet and WiFi MAC address and Bluetooth BD_ADDR comes up many many times. Why not have something generic here. And don't tell me request_firmware is that generic solution ;)
Regards
Marcel
^ permalink raw reply
* Re: [PATCH net] netlink: use jhash as hashfn for rhashtable
From: Hannes Frederic Sowa @ 2014-12-08 16:56 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, netdev, Herbert Xu, Thomas Graf
In-Reply-To: <1418056230-8700-1-git-send-email-dborkman@redhat.com>
On Mo, 2014-12-08 at 17:30 +0100, Daniel Borkmann wrote:
> For netlink, we shouldn't be using arch_fast_hash() as a hashing
> discipline, but rather jhash() instead.
>
> Since netlink sockets can be opened by any user, a local attacker
> would be able to easily create collisions with the DPDK-derived
> arch_fast_hash(), which trades off performance for security by
> using crc32 CPU instructions on x86_64.
>
> While it might have a legimite use case in other places, it should
> be avoided in netlink context, though. As rhashtable's API is very
> flexible, we could later on still decide on other hashing disciplines,
> if legitimate.
>
> Reference: http://thread.gmane.org/gmane.linux.kernel/1844123
> Fixes: e341694e3eb5 ("netlink: Convert netlink_lookup() to use RCU protected hash table")
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Thomas Graf <tgraf@suug.ch>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
> net/netlink/af_netlink.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 0007b81..b6bf8e8 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -3130,7 +3130,7 @@ static int __init netlink_proto_init(void)
> .head_offset = offsetof(struct netlink_sock, node),
> .key_offset = offsetof(struct netlink_sock, portid),
> .key_len = sizeof(u32), /* portid */
> - .hashfn = arch_fast_hash,
> + .hashfn = jhash,
> .max_shift = 16, /* 64K */
> .grow_decision = rht_grow_above_75,
> .shrink_decision = rht_shrink_below_30,
Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
In net-next, some time soon, we should try to let all function pointers
to jhash() use one non-inline version. The other arch_fast_hash patch
adds __jhash for x86-only, we can move it over to lib/.
Thanks,
Hannes
^ permalink raw reply
* Re: [PATCH][net-next] net: avoid to call skb_queue_len again
From: Sergei Shtylyov @ 2014-12-08 16:51 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Li RongQing, netdev
In-Reply-To: <1418051436.15618.43.camel@edumazet-glaptop2.roam.corp.google.com>
Hello.
On 12/08/2014 06:10 PM, Eric Dumazet wrote:
>> I expect you to also refine the description, so that it's meaningful,
>> unlike now.
> It seems obvious to me Li is not a native English speaker.
Me neither. :-)
However, the good command of English language was a requirement when I was
first hired to do the Linux development.
> I understood
> the patch very well, and the changelog seemed fine to me.
Oh, if you say so...
> What about you provide this description instead, since you seem to care
> very much ?
I mostly care for others; I don't suppose much people except you are able
to understand the current variant. And I now have neither enough time nor
enough understanding to write a proper description for this patch. I can only
suggest that you refine the description for others if you can understand it so
well.
> Thanks !
Not at all.
WBR, Sergei
^ permalink raw reply
* Re: wl1251: NVS firmware data
From: Pali Rohár @ 2014-12-08 16:47 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Ming Lei, Pavel Machek, John W. Linville, Grazvydas Ignotas,
linux-wireless@vger.kernel.org, Network Development,
Linux Kernel Mailing List, Ivaylo Dimitrov, Aaro Koskinen,
Kalle Valo, Sebastian Reichel, David Gnedt
In-Reply-To: <20141208163714.GA31169@kroah.com>
[-- Attachment #1: Type: Text/Plain, Size: 1991 bytes --]
On Monday 08 December 2014 17:37:14 Greg Kroah-Hartman wrote:
> On Mon, Dec 08, 2014 at 11:18:18PM +0800, Ming Lei wrote:
> > On Sat, Dec 6, 2014 at 9:02 PM, Pali Rohár
<pali.rohar@gmail.com> wrote:
> > > On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
> > > /**
> > >
> > > + * request_firmware_prefer_user: - prefer usermode helper
> > > for loading firmware + * @firmware_p: pointer to firmware
> > > image
> > > + * @name: name of firmware file
> > > + * @device: device for which firmware is being loaded
> > > + *
> > > + * This function works pretty much like
> > > request_firmware(), but it prefer + * usermode helper. If
> > > usermode helper fails then it fallback to direct access.
> > > + * Usefull for dynamic or model specific firmware data.
> > > + **/
> > > +int request_firmware_prefer_user(const struct firmware
> > > **firmware_p, + const char
> > > *name, struct device *device) +{
> > > + int ret;
> > > + __module_get(THIS_MODULE);
> > > + ret = _request_firmware(firmware_p, name, device,
> > > + FW_OPT_UEVENT |
> > > FW_OPT_PREFER_USER); + module_put(THIS_MODULE);
> > > + return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
> >
> > I'd like to introduce request_firmware_user() which only
> > requests firmware from user space, and this way is simpler
> > and more flexible since we have request_firmware_direct()
> > already.
>
> Why would a driver care about what program provides the
> firmware? It shouldn't at all, and we want to get rid of the
> userspace firmware loader, not encourage drivers to use it
> "exclusively" at all.
>
Do not remove it! Without userspace firmware loader it is
impossible to load dynamic firmware files.
> So no, I don't want to see this, and I don't want drivers to
> worry about this either.
>
> greg k-h
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [RFC][PATCHES] iov_iter.c rewrite
From: Kirill A. Shutemov @ 2014-12-08 16:46 UTC (permalink / raw)
To: Al Viro; +Cc: Linus Torvalds, linux-kernel, linux-fsdevel, netdev
In-Reply-To: <20141204202011.GO29748@ZenIV.linux.org.uk>
On Thu, Dec 04, 2014 at 08:20:11PM +0000, Al Viro wrote:
> First of all, I want to apologize for the nastiness of preprocessor
> use in this series. Seeing that the whole "macros that look like new kinds
> of C statements" thing (including list_for_each_...(), etc) is very much not
> to my liking, I really don't trust my taste on finer details and I'd very
> much like some feedback.
>
> The reason for doing that kind of tricks is that iov_iter.c keeps
> growing more and more boilerplate code. For iov_iter-net series we need
> * csum_and_copy_from_iter()
> * csum_and_copy_to_iter()
> * copy_from_iter_nocache()
> That's 3 new primitives, each in 2 variants (iovec and bvec).
> * ITER_KVEC handled without going through uaccess.h stuff (and
> independent of set_fs() state).
> And *that* means 3 variants intstead of 2 for most of the existing primitives.
> That's far too much, and the amount of copies of the same logics would pretty
> much guarantee that it will be a breeding ground for hard-to-kill bugs.
>
> The following series (also in vfs.git#iov_iter) actually manages to
> do all of the above *and* shrink the damn thing quite a bit. The generated
> code appears to be no worse than before. The price is a couple of iterator
> macros - iterate_all_kinds() and iterate_and_advance(). They are given an
> iov_iter, size (i.e. the amount of data in iov_iter beginning we want to go
> through), name of the loop variable and 3 variants of loop body - for iovec,
> bvec and kvec resp. Loop variable is declared *inside* the expansion of those
> suckers according to the kind of iov_iter - it's struct iovec, struct bio_vec
> or struct kvec, covering the current range to deal with.
> The difference between those two is that iterate_and_advance() will
> advance the iov_iter by the amount it has handled and iterate_all_kinds()
> will leave iov_iter unchanged.
>
> Unless I hear anybody yelling, it goes into vfs.git#for-next today,
> so if you have objections, suggestions, etc., give those *now*.
>
> Al Viro (13):
> iov_iter.c: macros for iterating over iov_iter
> iov_iter.c: iterate_and_advance
> iov_iter.c: convert iov_iter_npages() to iterate_all_kinds
> iov_iter.c: convert iov_iter_get_pages() to iterate_all_kinds
> iov_iter.c: convert iov_iter_get_pages_alloc() to iterate_all_kinds
> iov_iter.c: convert iov_iter_zero() to iterate_and_advance
> iov_iter.c: get rid of bvec_copy_page_{to,from}_iter()
> iov_iter.c: convert copy_from_iter() to iterate_and_advance
> iov_iter.c: convert copy_to_iter() to iterate_and_advance
> iov_iter.c: handle ITER_KVEC directly
> csum_and_copy_..._iter()
> new helper: iov_iter_kvec()
> copy_from_iter_nocache()
I guess this crash is related to the patchset.
[ 102.337742] ------------[ cut here ]------------
[ 102.338270] kernel BUG at /home/kas/git/public/linux-next/arch/x86/mm/physaddr.c:26!
[ 102.339043] invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC
[ 102.339622] Modules linked in:
[ 102.339951] CPU: 2 PID: 6029 Comm: trinity-c23 Not tainted 3.18.0-next-20141208-00036-gc7edb4791544-dirty #269
[ 102.340011] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
[ 102.340011] task: ffff880041c51510 ti: ffff880049c70000 task.ti: ffff880049c70000
[ 102.340011] RIP: 0010:[<ffffffff8104d8c0>] [<ffffffff8104d8c0>] __phys_addr+0x40/0x60
[ 102.340011] RSP: 0018:ffff880049c73838 EFLAGS: 00010206
[ 102.340011] RAX: 00004100174b4000 RBX: ffff880049c73b08 RCX: 0000000000000028
[ 102.340011] RDX: 0000000000000041 RSI: ffff88015dc980a8 RDI: ffffc900174b4000
[ 102.340011] RBP: ffff880049c73838 R08: ffffc900174b4000 R09: 000000000000000c
[ 102.340011] R10: 0000000000000000 R11: 0000000000000000 R12: ffff88015dc980a8
[ 102.340011] R13: ffffc900174f4000 R14: ffffea0000000000 R15: ffffc900174b4000
[ 102.340011] FS: 00007fcdd37fb700(0000) GS:ffff88017aa00000(0000) knlGS:0000000000000000
[ 102.340011] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 102.340011] CR2: 00000000006246a8 CR3: 0000000068dbb000 CR4: 00000000001406e0
[ 102.340011] Stack:
[ 102.340011] ffff880049c73888 ffffffff8117a96b 0000000000000002 0000000000040000
[ 102.340011] ffffffff81204b3f ffff88015dc98028 0000000000000000 ffff880049c73a78
[ 102.340011] ffff88015dc98000 ffff880049c73a10 ffff880049c73978 ffffffff81203ec9
[ 102.340011] Call Trace:
[ 102.340011] [<ffffffff8117a96b>] iov_iter_get_pages+0x17b/0x390
[ 102.340011] [<ffffffff81204b3f>] ? __blockdev_direct_IO+0x15f/0x16a0
[ 102.340011] [<ffffffff81203ec9>] do_direct_IO+0x10a9/0x1bc0
[ 102.340011] [<ffffffff810a92d8>] ? lockdep_init_map+0x68/0x5c0
[ 102.340011] [<ffffffff81204d6c>] __blockdev_direct_IO+0x38c/0x16a0
[ 102.340011] [<ffffffff810a9d27>] ? __lock_acquire+0x4f7/0xd40
[ 102.340011] [<ffffffff810a73f9>] ? mark_held_locks+0x79/0xb0
[ 102.340011] [<ffffffff8133b510>] ? xfs_get_blocks+0x20/0x20
[ 102.340011] [<ffffffff81338ff0>] xfs_vm_direct_IO+0x120/0x140
[ 102.340011] [<ffffffff8133b510>] ? xfs_get_blocks+0x20/0x20
[ 102.340011] [<ffffffff810aa773>] ? lock_release_non_nested+0x203/0x3d0
[ 102.340011] [<ffffffff8135b9a7>] ? xfs_ilock+0x167/0x2e0
[ 102.340011] [<ffffffff8114d957>] generic_file_read_iter+0x517/0x6a0
[ 102.340011] [<ffffffff810a73f9>] ? mark_held_locks+0x79/0xb0
[ 102.340011] [<ffffffff8185e192>] ? __mutex_unlock_slowpath+0xb2/0x190
[ 102.340011] [<ffffffff8134bc2f>] xfs_file_read_iter+0x12f/0x460
[ 102.340011] [<ffffffff811c237e>] new_sync_read+0x7e/0xb0
[ 102.340011] [<ffffffff811c3528>] __vfs_read+0x18/0x50
[ 102.340011] [<ffffffff811c35ed>] vfs_read+0x8d/0x150
[ 102.340011] [<ffffffff811c89e8>] kernel_read+0x48/0x60
[ 102.340011] [<ffffffff810f4a92>] copy_module_from_fd.isra.51+0x112/0x170
[ 102.340011] [<ffffffff810f7646>] SyS_finit_module+0x56/0x80
[ 102.340011] [<ffffffff81861f92>] system_call_fastpath+0x12/0x17
[ 102.340011] Code: 00 00 00 00 00 78 00 00 48 01 f8 48 39 c2 72 1b 0f b6 0d 9d 7a ec 00 48 89 c2 48 d3 ea 48 85 d2 75 09 5d c3 0f 1f 80 00 00 00 00 <0f> 0b 66 0f 1f 44 00 00 48 89 d0 48 03 05 3e 87 dc 00 48 81 fa
[ 102.340011] RIP [<ffffffff8104d8c0>] __phys_addr+0x40/0x60
[ 102.340011] RSP <ffff880049c73838>
[ 102.371939] ---[ end trace e07368268cd6b49c ]---
--
Kirill A. Shutemov
^ permalink raw reply
* Re: Q: need effective backlog for listen()
From: Philippe Troin @ 2014-12-08 16:35 UTC (permalink / raw)
To: Ulrich Windl; +Cc: netdev
In-Reply-To: <5485ACC9020000A100018394@gwsmtp1.uni-regensburg.de>
On Mon, 2014-12-08 at 13:51 +0100, Ulrich Windl wrote:
> (not subscribed to the list, plese keep me on CC:)
>
> I have a problem I could not find the answer. I suspect the problem
> arises from Linux derivating from standard functionality...
>
> I have written a server that should accept n TCP connections at most.
> I was expecting that the backlog parameter of listen will cause extra
> connection requests either
> 1) to be refused
> or
> 2) to time out eventually
>
> (The standard seems to say that extra connections are refused)
The argument to listen() specifies how many connections the system is
allow to keep waiting to be accept()ed.
As soon as you accept() the connection, the count is decremented.
So that won't help for your use case.
> However none of the above see ms true. Even if my server delays
> accept()ing new connections, no client ever sees a "connection
> refused" or "connection timed out". Is there any chance to signal the
> client that no more connections are accepted at the moment?
Close the listening socket. No new connections will be accepted.
When you reopen the socket for accepting new connections, you may have
to use SO_REUSEADDR before bind()ing to the port.
Phil.
^ permalink raw reply
* Re: [PATCH net-next 2/3] netlink: IFLA_PHYS_SWITCH_ID to IFLA_PHYS_PARENT_ID
From: Jiri Pirko @ 2014-12-08 16:41 UTC (permalink / raw)
To: Andy Gospodarek; +Cc: netdev, sfeldma
In-Reply-To: <20141208153747.GG797@gospo.home.greyhouse.net>
Mon, Dec 08, 2014 at 04:37:47PM CET, gospo@cumulusnetworks.com wrote:
>On Mon, Dec 08, 2014 at 04:17:14PM +0100, Jiri Pirko wrote:
>> Fri, Dec 05, 2014 at 07:02:16PM CET, gospo@cumulusnetworks.com wrote:
>> >There has been much discussion about proper nomenclature to use for this
>> >and I would prefer parent rather than calling every forwarding element a
>> >switch.
>>
>> Andy, I must say I really do not like just plain "parent". It is really
>> not clear what it means as it can mean 1000 things.
>>
>> I know "switch" is not ideal but everytime anyone is talking about these
>> kind of forwarding devices, they use word "switch" even if it is not
>> accurate and everyone knows what they are talking about. Nobody uses
>> "parent".
>
>Well of course they are not going to use it until it's committed. ;-)
Do you seriously expect people talking about "parents" instead of
"switches". I doubt that...
>
>> For me this is nack for this patchset.
>
>Thanks for the review. I am not big marketing person, so it was not
>clear to me what was ideal. Due to parent already being in the code
>and having as a logical description of the relationship (parent
>switch/router device and sibling network interfaces -- like sibling CPU
>cores on the same socket).
>
>I do really want to collectively come up with something other than
>switch for everything. Those L3 ops with 'switch' in the name will
>feel really awkward....
I say it is not optimal, but I did not see any better proposal...
>
>>
>> Jiri
>>
>> >
>> >Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com>
>> >---
>> > include/uapi/linux/if_link.h | 2 +-
>> > net/core/rtnetlink.c | 4 ++--
>> > 2 files changed, 3 insertions(+), 3 deletions(-)
>> >
>> >diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>> >index f7d0d2d..3d8edd8 100644
>> >--- a/include/uapi/linux/if_link.h
>> >+++ b/include/uapi/linux/if_link.h
>> >@@ -145,7 +145,7 @@ enum {
>> > IFLA_CARRIER,
>> > IFLA_PHYS_PORT_ID,
>> > IFLA_CARRIER_CHANGES,
>> >- IFLA_PHYS_SWITCH_ID,
>> >+ IFLA_PHYS_PARENT_ID,
>> > __IFLA_MAX
>> > };
>> >
>> >diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> >index 61cb7e7..1fe0a16 100644
>> >--- a/net/core/rtnetlink.c
>> >+++ b/net/core/rtnetlink.c
>> >@@ -982,7 +982,7 @@ static int rtnl_phys_switch_id_fill(struct sk_buff *skb, struct net_device *dev)
>> > return err;
>> > }
>> >
>> >- if (nla_put(skb, IFLA_PHYS_SWITCH_ID, psid.id_len, psid.id))
>> >+ if (nla_put(skb, IFLA_PHYS_PARENT_ID, psid.id_len, psid.id))
>> > return -EMSGSIZE;
>> >
>> > return 0;
>> >@@ -1222,7 +1222,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
>> > [IFLA_NUM_RX_QUEUES] = { .type = NLA_U32 },
>> > [IFLA_PHYS_PORT_ID] = { .type = NLA_BINARY, .len = MAX_PHYS_ITEM_ID_LEN },
>> > [IFLA_CARRIER_CHANGES] = { .type = NLA_U32 }, /* ignored */
>> >- [IFLA_PHYS_SWITCH_ID] = { .type = NLA_BINARY, .len = MAX_PHYS_ITEM_ID_LEN },
>> >+ [IFLA_PHYS_PARENT_ID] = { .type = NLA_BINARY, .len = MAX_PHYS_ITEM_ID_LEN },
>> > };
>> >
>> > static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
>> >--
>> >1.9.3
>> >
>--
>To unsubscribe from this list: send the line "unsubscribe netdev" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net] netlink: use jhash as hashfn for rhashtable
From: Thomas Graf @ 2014-12-08 16:38 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, netdev, Herbert Xu, Hannes Frederic Sowa
In-Reply-To: <1418056230-8700-1-git-send-email-dborkman@redhat.com>
On 12/08/14 at 05:30pm, Daniel Borkmann wrote:
> For netlink, we shouldn't be using arch_fast_hash() as a hashing
> discipline, but rather jhash() instead.
>
> Since netlink sockets can be opened by any user, a local attacker
> would be able to easily create collisions with the DPDK-derived
> arch_fast_hash(), which trades off performance for security by
> using crc32 CPU instructions on x86_64.
>
> While it might have a legimite use case in other places, it should
> be avoided in netlink context, though. As rhashtable's API is very
> flexible, we could later on still decide on other hashing disciplines,
> if legitimate.
>
> Reference: http://thread.gmane.org/gmane.linux.kernel/1844123
> Fixes: e341694e3eb5 ("netlink: Convert netlink_lookup() to use RCU protected hash table")
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Thomas Graf <tgraf@suug.ch>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Acked-by: Thomas Graf <tgraf@suug.ch>
^ permalink raw reply
* Re: wl1251: NVS firmware data
From: Greg Kroah-Hartman @ 2014-12-08 16:37 UTC (permalink / raw)
To: Ming Lei
Cc: Pali Rohár, Pavel Machek, John W. Linville,
Grazvydas Ignotas, linux-wireless@vger.kernel.org,
Network Development, Linux Kernel Mailing List, Ivaylo Dimitrov,
Aaro Koskinen, Kalle Valo, Sebastian Reichel, David Gnedt
In-Reply-To: <CACVXFVPOLfDuqc0nLb-zM8vH618DLXy0xtZbUOn5_XvdxRZSDw@mail.gmail.com>
On Mon, Dec 08, 2014 at 11:18:18PM +0800, Ming Lei wrote:
> On Sat, Dec 6, 2014 at 9:02 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > On Saturday 06 December 2014 13:49:54 Pavel Machek wrote:
>
> >
> > /**
> > + * request_firmware_prefer_user: - prefer usermode helper for loading firmware
> > + * @firmware_p: pointer to firmware image
> > + * @name: name of firmware file
> > + * @device: device for which firmware is being loaded
> > + *
> > + * This function works pretty much like request_firmware(), but it prefer
> > + * usermode helper. If usermode helper fails then it fallback to direct access.
> > + * Usefull for dynamic or model specific firmware data.
> > + **/
> > +int request_firmware_prefer_user(const struct firmware **firmware_p,
> > + const char *name, struct device *device)
> > +{
> > + int ret;
> > + __module_get(THIS_MODULE);
> > + ret = _request_firmware(firmware_p, name, device,
> > + FW_OPT_UEVENT | FW_OPT_PREFER_USER);
> > + module_put(THIS_MODULE);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
>
> I'd like to introduce request_firmware_user() which only requests
> firmware from user space, and this way is simpler and more flexible
> since we have request_firmware_direct() already.
Why would a driver care about what program provides the firmware? It
shouldn't at all, and we want to get rid of the userspace firmware
loader, not encourage drivers to use it "exclusively" at all.
So no, I don't want to see this, and I don't want drivers to worry about
this either.
greg k-h
^ permalink raw reply
* [PATCH net] netlink: use jhash as hashfn for rhashtable
From: Daniel Borkmann @ 2014-12-08 16:30 UTC (permalink / raw)
To: davem; +Cc: netdev, Herbert Xu, Thomas Graf, Hannes Frederic Sowa
For netlink, we shouldn't be using arch_fast_hash() as a hashing
discipline, but rather jhash() instead.
Since netlink sockets can be opened by any user, a local attacker
would be able to easily create collisions with the DPDK-derived
arch_fast_hash(), which trades off performance for security by
using crc32 CPU instructions on x86_64.
While it might have a legimite use case in other places, it should
be avoided in netlink context, though. As rhashtable's API is very
flexible, we could later on still decide on other hashing disciplines,
if legitimate.
Reference: http://thread.gmane.org/gmane.linux.kernel/1844123
Fixes: e341694e3eb5 ("netlink: Convert netlink_lookup() to use RCU protected hash table")
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Thomas Graf <tgraf@suug.ch>
Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
net/netlink/af_netlink.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 0007b81..b6bf8e8 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -3130,7 +3130,7 @@ static int __init netlink_proto_init(void)
.head_offset = offsetof(struct netlink_sock, node),
.key_offset = offsetof(struct netlink_sock, portid),
.key_len = sizeof(u32), /* portid */
- .hashfn = arch_fast_hash,
+ .hashfn = jhash,
.max_shift = 16, /* 64K */
.grow_decision = rht_grow_above_75,
.shrink_decision = rht_shrink_below_30,
--
1.7.11.7
^ permalink raw reply related
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