* Re: [PATCH] openvswitch: Fix alignment of struct sw_flow_key.
From: David Miller @ 2013-09-05 19:49 UTC (permalink / raw)
To: geert; +Cc: jesse, netdev, dev, azhou, fengguan.wu
In-Reply-To: <CAMuHMdVgd499_oAU0oodE8ie+h+Oc6M-dRcyC4tT6QDB=01GSg@mail.gmail.com>
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Thu, 5 Sep 2013 21:20:16 +0200
> Why don't you abort the loop if a difference is found?
We are optimizing for a match.
^ permalink raw reply
* Re: [PATCH net] net: netlink: filter particular protocols from analyzers
From: Daniel Borkmann @ 2013-09-05 19:48 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Stephen Hemminger
In-Reply-To: <20130905.144442.2085221662776542385.davem@davemloft.net>
On 09/05/2013 08:44 PM, David Miller wrote:
> From: Daniel Borkmann <dborkman@redhat.com>
> Date: Thu, 5 Sep 2013 17:48:47 +0200
>
>> From: Daniel Borkmann <dborkmann@redhat.com>
>>
>> Fix finer-grained control and let only a whitelist of allowed netlink
>> protocols pass, in our case related to networking. If later on, other
>> subsystems decide they want to add their protocol as well to the list
>> of allowed protocols they shall simply add it. While at it, we also
>> need to tell what protocol is in use otherwise BPF_S_ANC_PROTOCOL can
>> not pick it up (as it's not filled out).
>>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
To answer Stephen and Dave in one message ... ;-)
> This takes away functionality that I'd be more interesting in using,
> namely being able to listen to all netlink protocols using one tap.
>
> Seriously, when I first saw this feature, that was the first way I'd
> imagine myself using it, as a tcpdump for netlink traffic, all of
> it.
>
> If I just want to hear all netlink traffic, don't make me be forced to
> know every single NETLINK_* protocol value and have to open that many
> sockets just to do so.
>
> It also makes it so that I can't listen to userlevel custom netlink
> protocols, another minus of filtering.
>
> At the very least, allow an sk_protocol of zero or similar to have this
> meaning of "everything".
I agree with you with all the above and I think with having this in
tcpdump would be great to debug netlink traffic of course ...
With socket(PF_PACKET, ..., htons(ETH_P_ALL)) you will already get all
users from the suggested white-list of the patch, which is the majority
of netlink users I believe. Hence, you do not need to have one socket
per protocol. skbs from there should get dragged into pf_packet via
dev_queue_xmit_nit() which works on ptype_all list.
Plus, with the nskb->protocol addition in this patch, they can then either
take all netlink traffic from that, or filter with BPF e.g. for particular
protocols via BPF_S_ANC_PROTOCOL or more advanced stuff, for example. Also,
they can select particular dissectors with that information that comes in
via sll_protocol which is useful, of course.
I think for out-of-tree modules, we never really cared much, and they
could use generic netlink probably anyway. The thing why I wanted to
do this is to keep security related messages (audit, selinux) generally
under the radar, which is "more or less" what's remaining, so we still get
main users. This patch would accomplish those things, that's why I think
it's useful.
^ permalink raw reply
* Re: [PATCH] openvswitch: Fix alignment of struct sw_flow_key.
From: Joe Perches @ 2013-09-05 19:47 UTC (permalink / raw)
To: David Miller; +Cc: jesse, netdev, dev, azhou, fengguan.wu, geert
In-Reply-To: <20130905.144044.1053960608071929025.davem@davemloft.net>
On Thu, 2013-09-05 at 14:40 -0400, David Miller wrote:
> From: Jesse Gross <jesse@nicira.com>
> Date: Thu, 5 Sep 2013 11:36:19 -0700
> > On Thu, Sep 5, 2013 at 11:17 AM, David Miller <davem@davemloft.net> wrote:
> >> From: Jesse Gross <jesse@nicira.com>
> >> Date: Thu, 5 Sep 2013 10:41:27 -0700
> >>
> >>> -} __aligned(__alignof__(long));
> >>> +} __aligned(8); /* 8 byte alignment ensures this can be accessed as a long */
> >>
> >> This kind of stuff drives me crazy.
> >>
> >> If the issue is the type, therefore at least use an expression that
> >> mentions the type explicitly. And mention the actual type that
> >> matters. "long" isn't it.
> >
> > 'long' actually is the real type here.
> >
> > When doing comparisons, this structure is being accessed as a byte
> > array in 'long' sized chunks, not by its members. Therefore, the
> > compiler's alignment does not necessarily correspond to anything for
> > this purpose. It could be a struct full of u16's and we would still
> > want to access it in chunks of 'long'.
> >
> > To completely honest, I think the correct alignment should be
> > sizeof(long) because I know that 'long' is not always 8 bytes on all
> > architectures. However, you made the point before that this could
> > break the alignment of the 64-bit values on architectures where 'long'
> > is 32 bits wide, so 8 bytes is the generic solution.
>
> Look at net/core/flow.c:flow_key_compare().
>
> And then we annotate struct flowi with
>
> } __attribute__((__aligned__(BITS_PER_LONG/8)));
>
> Don't reinvent the wheel, either mimick how existing code does
> this kind of thing or provide a justification for doing it differently
> and update the existing cases to match and be consistent.
I think using the BITS_PER_LONG #define is pretty odd
as it's set using a test for CONFIG_64BIT and that
__aligned(__alignof__(long))
or
__aligned(sizeof(long))
may be simpler to read. That first would allow
architectures that can read any long on an arbitrary
boundary to pack the structure as densely as possible.
That may not work if the entire structure is always
read with via long *.
If so, my choice would be to standardize using
} __aligned(sizeof(long));
Currently, what happens to a struct that isn't
actually a multiple of long bytes? Are these
structs guaranteed to be zero padded right now?
Also, what happens for structs like:
struct foo {
u8 bar[6];
long baz;
u8 qux[2];
} __aligned(sizeof(long));
where the padding may or may not exist if
__packed is also specified?
btw:
all the __attribute__((__aligned__(foo)...)) and
__aligned(...) / __packed(...) uses could be rewritten
to a more standard style.
$ grep -rP --include=*.[ch] -oh "\b__aligned[^\(\n_]*\s*\([^;\n]+[;\n]" * | \
sed -r -e 's/\s//g' -e 's/\)[^\)]*$/\)/' | sort | uniq -c
3 __aligned(1)
3 __aligned(16)
1 __aligned(1<<WORK_STRUCT_FLAG_BITS)
12 __aligned(2)
7 __aligned(32)
32 __aligned(4)
8 __aligned(64)
15 __aligned(8)
1 __aligned(__alignof__(long))
3 __aligned(__CACHE_WRITEBACK_GRANULE)
1 __aligned((IOR_DMA_OFFSET_BITS/IOR_BPC))
1 __aligned((IOR_PHYS_OFFSET_BITS/IOR_BPC))
1 __aligned(LOG_ALIGN)
2 __aligned(NETDEV_ALIGN)
10 __aligned(PAGE_SIZE)
2 __aligned(sizeof(s64))
4 __aligned(sizeof(u16))
2 __aligned(sizeof(u32))
7 __aligned(sizeof(void*))
$ grep -rP --include=*.[ch] -oh "\b__attribute.*aligned[^\(\n]*\s*\([^;\n]+[;\n]" * | \
sed -r -e 's/\s//g' -e 's/\)[^\)]*$/\)/' | sort | uniq -c
2 __attribute__((aligned(0x100)))
1 __attribute__((aligned(0x2000)))
2 __attribute__((aligned(0x20000)))
1 __attribute__((__aligned__(0x400)))
3 __attribute__((aligned(0x80)))
1 __attribute__((aligned(1024),packed))
2 __attribute__((__aligned__(128)))
9 __attribute__((__aligned__(16)))
30 __attribute__((aligned(16)))
2 __attribute__((aligned(1),packed))
1 __attribute__((aligned(2)))
1 __attribute__((aligned(2048)))
14 __attribute__((aligned(256)))
1 __attribute__((aligned(256),packed))
5 __attribute__((aligned(2),packed))
1 __attribute__((aligned(2*sizeof(long))))
1 __attribute((aligned(2*sizeof(unsignedlong))))
3 __attribute__((__aligned__(32)))
53 __attribute__((aligned(32)))
2 __attribute__((aligned(32),packed))
2 __attribute__((__aligned__(4)))
22 __attribute__((aligned(4)))
2 __attribute__((aligned(4096)))
1 __attribute__((aligned(4<<10)))
1 __attribute__((aligned(4),packed))
18 __attribute__((aligned(64)))
1 __attribute__((aligned(65536)))
15 __attribute__((__aligned__(8)))
89 __attribute__((aligned(8)))
2 __attribute__((aligned(a)))
5 __attribute__((aligned(__alignof__(structebt_replace))))
1 __attribute__((aligned(__alignof__(structhash_alg_common))))
1 __attribute__((aligned(__alignof__(u32))))
1 __attribute__((aligned(ATOMIC_HASH_L2_SIZE*4)))
4 __attribute__((__aligned__(BITS_PER_LONG/8)))
1 __attribute__((aligned(BUFFER_SIZE)))
2 __attribute__((aligned(CHIP_L2_LINE_SIZE())))
1 __attribute__((aligned(CPPI_DESCRIPTOR_ALIGN)))
1 __attribute__((aligned(DM_IO_MAX_REGIONS)))
1 __attribute__((aligned(HV_PAGE_TABLE_ALIGN)))
1 __attribute__((aligned(_K_SS_ALIGNSIZE)))
1 __attribute__((aligned(L1_CACHE_BYTES)))
3 __attribute__((aligned(L2_CACHE_BYTES)))
1 __attribute__((__aligned__(NETDEV_ALIGN)))
3 __attribute__((__aligned__(PADLOCK_ALIGNMENT)))
4 __attribute__((__aligned__(PAGE_SIZE)))
7 __attribute__((aligned(PAGE_SIZE)))
1 __attribute__((aligned(PS3_BMP_MINALIGN)))
2 __attribute__((aligned(sizeof(Elf##size##_Word))))
1 __attribute__((aligned(sizeof(int))))
1 __attribute__((aligned(sizeof(__kernel_size_t))))
1 __attribute__((aligned(sizeof(kernel_ulong_t))))
7 __attribute__((aligned(sizeof(long))))
1 __attribute__((aligned(sizeof(s64))))
1 __attribute__((__aligned__(sizeof(structxencomm_mini))))
1 __attribute__((aligned(sizeof(__u64))))
8 __attribute__((aligned(sizeof(uint64_t))))
6 __attribute__((aligned(sizeof(unsignedlong))))
1 __attribute__((__aligned__(sizeof(void*))))
1 __attribute__((aligned(sizeof(void*))))
7 __attribute__((__aligned__(SMP_CACHE_BYTES)))
6 __attribute__((aligned(SMP_CACHE_BYTES)))
1 __attribute__((aligned(stackalign)))
1 __attribute__((aligned(THREAD_SIZE)))
1 __attribute__((aligned(TSB_ENTRY_ALIGNMENT)))
1 __attribute__((aligned(XCHAL_CP0_SA_ALIGN)))
1 __attribute__((aligned(XCHAL_CP1_SA_ALIGN)))
1 __attribute__((aligned(XCHAL_CP2_SA_ALIGN)))
1 __attribute__((aligned(XCHAL_CP3_SA_ALIGN)))
1 __attribute__((aligned(XCHAL_CP4_SA_ALIGN)))
1 __attribute__((aligned(XCHAL_CP5_SA_ALIGN)))
1 __attribute__((aligned(XCHAL_CP6_SA_ALIGN)))
1 __attribute__((aligned(XCHAL_CP7_SA_ALIGN)))
2 __attribute__((aligned(XCHAL_NCP_SA_ALIGN)))
1 __attribute__((packed,aligned(1024)))
1 __attribute__((packed,__aligned__(16)))
4 __attribute__((packed,aligned(16)))
7 __attribute__((packed,aligned(2)))
1 __attribute__((packed,aligned(2048)))
4 __attribute__((packed,aligned(256)))
177 __attribute__((packed,aligned(4)))
2 __attribute__((packed,aligned(4096)))
2 __attribute__((packed,aligned(64)))
47 __attribute__((packed,aligned(8)))
2 __attribute__((packed,aligned(PAGE_SIZE)))
1 __attribute__((packed,aligned(PMCRAID_IOADL_ALIGNMENT)))
2 __attribute__((packed,aligned(PMCRAID_IOARCB_ALIGNMENT)))
1 __attribute__((__section__(".data..vm0.pgd"),aligned(PAGE_SIZE)))
1 __attribute__((__section__(".data..vm0.pmd"),aligned(PAGE_SIZE)))
1 __attribute__((__section__(".data..vm0.pte"),aligned(PAGE_SIZE)))
^ permalink raw reply
* Re: [PATCH net] net: netlink: filter particular protocols from analyzers
From: Stephen Hemminger @ 2013-09-05 19:44 UTC (permalink / raw)
To: David Miller; +Cc: dborkman, netdev, dborkmann
In-Reply-To: <20130905.150341.1090416835512463609.davem@davemloft.net>
On Thu, 05 Sep 2013 15:03:41 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:
> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Thu, 5 Sep 2013 11:57:55 -0700
>
> > If you want filtering, why not add BPF (sk_filter) support to this?
>
> That's one idea.
>
> But using sk_protocol is at least consistent with how AF_PACKET does
> things.
Why not both.
^ permalink raw reply
* Re: [PATCH] openvswitch: Fix alignment of struct sw_flow_key.
From: Jesse Gross @ 2013-09-05 19:25 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: David Miller, netdev@vger.kernel.org, dev@openvswitch.org,
Andy Zhou, fengguan.wu
In-Reply-To: <CAMuHMdVgd499_oAU0oodE8ie+h+Oc6M-dRcyC4tT6QDB=01GSg@mail.gmail.com>
On Thu, Sep 5, 2013 at 12:20 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Why don't you abort the loop if a difference is found?
> Or is this a security-related struct where you want to protect against
> timing attacks?
It's more expensive to test for a difference on every iteration in the
common case where the comparison succeeds.
> Furthermore, as you compare the raw bytes, I hope you always
> initialize all gaps in the struct to zero.
> E.g. there's a 2-byte gap immediately after "ip", as the next member
> is 32-bit (except op m68k, where the 32-bit member will be 2-byte aligned).
It's initialized to zero.
^ permalink raw reply
* Re: [PATCH] openvswitch: Fix alignment of struct sw_flow_key.
From: Geert Uytterhoeven @ 2013-09-05 19:25 UTC (permalink / raw)
To: Jesse Gross
Cc: David Miller, netdev@vger.kernel.org, dev@openvswitch.org,
Andy Zhou, Fengguang Wu
In-Reply-To: <1378408625-18415-1-git-send-email-jesse@nicira.com>
On Thu, Sep 5, 2013 at 9:17 PM, Jesse Gross <jesse@nicira.com> wrote:
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -1981,6 +1981,7 @@ nla_put_failure:
> * Returns zero if successful or a negative error code. */
> int ovs_flow_init(void)
> {
> + BUILD_BUG_ON(__alignof__(struct sw_flow_key) % __alignof__(long));
> -} __aligned(__alignof__(long));
> +} __aligned(BITS_PER_LONG/8); /* Ensure that we can do comparisons as longs. */
These don't match: the struct definition says aligned to 4 or 8 bytes, the check
checks for a multiple of the alignment of "long", which is 2, 4 or 8.
Anyway, BITS_PER_LONG/8 is always a multiple of __alignof__(long), so your
check will never fail.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH] openvswitch: Fix alignment of struct sw_flow_key.
From: Geert Uytterhoeven @ 2013-09-05 19:20 UTC (permalink / raw)
To: David Miller
Cc: Jesse Gross, netdev@vger.kernel.org, dev@openvswitch.org,
Andy Zhou, fengguan.wu
In-Reply-To: <20130905.144044.1053960608071929025.davem@davemloft.net>
On Thu, Sep 5, 2013 at 8:40 PM, David Miller <davem@davemloft.net> wrote:
> When doing comparisons, this structure is being accessed as a byte
> array in 'long' sized chunks, not by its members. Therefore, the
Like ... an optimized memcmp() does? Which handles all (un)alignment
well?
>> To completely honest, I think the correct alignment should be
>> sizeof(long) because I know that 'long' is not always 8 bytes on all
>> architectures. However, you made the point before that this could
>> break the alignment of the 64-bit values on architectures where 'long'
>> is 32 bits wide, so 8 bytes is the generic solution.
>
> Look at net/core/flow.c:flow_key_compare().
>
> And then we annotate struct flowi with
>
> } __attribute__((__aligned__(BITS_PER_LONG/8)));
>
> Don't reinvent the wheel, either mimick how existing code does
> this kind of thing or provide a justification for doing it differently
> and update the existing cases to match and be consistent.
This may still break the alignment of 64-bit values on 32-bit architectures
where __alignof__(u64) == 8.
Now let's look at the comparison function:
static bool __cmp_key(const struct sw_flow_key *key1,
const struct sw_flow_key *key2, int key_start, int key_end)
{
const long *cp1 = (long *)((u8 *)key1 + key_start);
const long *cp2 = (long *)((u8 *)key2 + key_start);
long diffs = 0;
int i;
for (i = key_start; i < key_end; i += sizeof(long))
diffs |= *cp1++ ^ *cp2++;
return diffs == 0;
}
Why don't you abort the loop if a difference is found?
Or is this a security-related struct where you want to protect against
timing attacks?
Furthermore, as you compare the raw bytes, I hope you always
initialize all gaps in the struct to zero.
E.g. there's a 2-byte gap immediately after "ip", as the next member
is 32-bit (except op m68k, where the 32-bit member will be 2-byte aligned).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH] openvswitch: Fix alignment of struct sw_flow_key.
From: Jesse Gross @ 2013-09-05 19:17 UTC (permalink / raw)
To: David Miller
Cc: netdev, dev, Andy Zhou, Fengguang Wu, Geert Uytterhoeven,
Jesse Gross
sw_flow_key alignment was declared as " __aligned(__alignof__(long))".
However, this breaks on the m68k architecture where long is 32 bit in
size but 16 bit aligned by default. This aligns to the size of a long to
ensure that we can always do comparsions in full long-sized chunks. It
also adds an additional build check to catch any reduction in alignment.
CC: Andy Zhou <azhou@nicira.com>
Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Jesse Gross <jesse@nicira.com>
---
net/openvswitch/flow.c | 1 +
net/openvswitch/flow.h | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index ad1aeeb..fb36f85 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -1981,6 +1981,7 @@ nla_put_failure:
* Returns zero if successful or a negative error code. */
int ovs_flow_init(void)
{
+ BUILD_BUG_ON(__alignof__(struct sw_flow_key) % __alignof__(long));
BUILD_BUG_ON(sizeof(struct sw_flow_key) % sizeof(long));
flow_cache = kmem_cache_create("sw_flow", sizeof(struct sw_flow), 0,
diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
index b65f885..212fbf7 100644
--- a/net/openvswitch/flow.h
+++ b/net/openvswitch/flow.h
@@ -125,7 +125,7 @@ struct sw_flow_key {
} nd;
} ipv6;
};
-} __aligned(__alignof__(long));
+} __aligned(BITS_PER_LONG/8); /* Ensure that we can do comparisons as longs. */
struct sw_flow {
struct rcu_head rcu;
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH] openvswitch: Fix alignment of struct sw_flow_key.
From: Jesse Gross @ 2013-09-05 19:14 UTC (permalink / raw)
To: David Miller
Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org, netdev,
Geert Uytterhoeven
In-Reply-To: <20130905.144044.1053960608071929025.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
On Thu, Sep 5, 2013 at 11:40 AM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> From: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
> Date: Thu, 5 Sep 2013 11:36:19 -0700
>
>> On Thu, Sep 5, 2013 at 11:17 AM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
>>> From: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
>>> Date: Thu, 5 Sep 2013 10:41:27 -0700
>>>
>>>> -} __aligned(__alignof__(long));
>>>> +} __aligned(8); /* 8 byte alignment ensures this can be accessed as a long */
>>>
>>> This kind of stuff drives me crazy.
>>>
>>> If the issue is the type, therefore at least use an expression that
>>> mentions the type explicitly. And mention the actual type that
>>> matters. "long" isn't it.
>>
>> 'long' actually is the real type here.
>>
>> When doing comparisons, this structure is being accessed as a byte
>> array in 'long' sized chunks, not by its members. Therefore, the
>> compiler's alignment does not necessarily correspond to anything for
>> this purpose. It could be a struct full of u16's and we would still
>> want to access it in chunks of 'long'.
>>
>> To completely honest, I think the correct alignment should be
>> sizeof(long) because I know that 'long' is not always 8 bytes on all
>> architectures. However, you made the point before that this could
>> break the alignment of the 64-bit values on architectures where 'long'
>> is 32 bits wide, so 8 bytes is the generic solution.
>
> Look at net/core/flow.c:flow_key_compare().
>
> And then we annotate struct flowi with
>
> } __attribute__((__aligned__(BITS_PER_LONG/8)));
>
> Don't reinvent the wheel, either mimick how existing code does
> this kind of thing or provide a justification for doing it differently
> and update the existing cases to match and be consistent.
Sure, I'll send a new patch to use BITS_PER_LONG.
^ permalink raw reply
* Re: [PATCH net] net: netlink: filter particular protocols from analyzers
From: David Miller @ 2013-09-05 19:03 UTC (permalink / raw)
To: stephen; +Cc: dborkman, netdev, dborkmann
In-Reply-To: <20130905115755.7aec533c@nehalam.linuxnetplumber.net>
From: Stephen Hemminger <stephen@networkplumber.org>
Date: Thu, 5 Sep 2013 11:57:55 -0700
> If you want filtering, why not add BPF (sk_filter) support to this?
That's one idea.
But using sk_protocol is at least consistent with how AF_PACKET does
things.
^ permalink raw reply
* Re: [PATCH net] net: netlink: filter particular protocols from analyzers
From: Stephen Hemminger @ 2013-09-05 18:57 UTC (permalink / raw)
To: David Miller; +Cc: dborkman, netdev, dborkmann
In-Reply-To: <20130905.144442.2085221662776542385.davem@davemloft.net>
On Thu, 05 Sep 2013 14:44:42 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:
> From: Daniel Borkmann <dborkman@redhat.com>
> Date: Thu, 5 Sep 2013 17:48:47 +0200
>
> > From: Daniel Borkmann <dborkmann@redhat.com>
> >
> > Fix finer-grained control and let only a whitelist of allowed netlink
> > protocols pass, in our case related to networking. If later on, other
> > subsystems decide they want to add their protocol as well to the list
> > of allowed protocols they shall simply add it. While at it, we also
> > need to tell what protocol is in use otherwise BPF_S_ANC_PROTOCOL can
> > not pick it up (as it's not filled out).
> >
> > Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>
> This takes away functionality that I'd be more interesting in using,
> namely being able to listen to all netlink protocols using one tap.
>
> Seriously, when I first saw this feature, that was the first way I'd
> imagine myself using it, as a tcpdump for netlink traffic, all of
> it.
>
> If I just want to hear all netlink traffic, don't make me be forced to
> know every single NETLINK_* protocol value and have to open that many
> sockets just to do so.
>
> It also makes it so that I can't listen to userlevel custom netlink
> protocols, another minus of filtering.
>
> At the very least, allow an sk_protocol of zero or similar to have this
> meaning of "everything".
>
> I'm not applying this patch, sorry.
> --
> 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
If you want filtering, why not add BPF (sk_filter) support to this?
^ permalink raw reply
* Re: [PATCH net] vxlan: Fix kernel panic on device delete.
From: David Miller @ 2013-09-05 18:51 UTC (permalink / raw)
To: pshelar; +Cc: netdev
In-Reply-To: <1378329141-11868-1-git-send-email-pshelar@nicira.com>
From: Pravin B Shelar <pshelar@nicira.com>
Date: Wed, 4 Sep 2013 14:12:21 -0700
> On vxlan device create if socket create fails vxlan device is not
> added to hash table. Therefore we need to check if device
> is in hashtable before we delete it from hlist.
> Following patch avoid the crash. net-next already has this fix.
...
> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
Applied.
^ permalink raw reply
* Re: [PATCH] net: mvneta: properly disable HW PHY polling and ensure adjust_link() works
From: David Miller @ 2013-09-05 18:51 UTC (permalink / raw)
To: jason
Cc: thomas.petazzoni, netdev, alior, jochen.armkernel, simon.guinot,
ryan, vdonnefort, ethan, stable, ezequiel.garcia, yves,
gregory.clement, psanford, w, linux-arm-kernel
In-Reply-To: <20130904145051.GO19598@titan.lakedaemon.net>
From: Jason Cooper <jason@lakedaemon.net>
Date: Wed, 4 Sep 2013 10:50:51 -0400
> On Wed, Sep 04, 2013 at 04:21:18PM +0200, Thomas Petazzoni wrote:
>> This commit fixes a long-standing bug that has been reported by many
>> users: on some Armada 370 platforms, only the network interface that
>> has been used in U-Boot to tftp the kernel works properly in
>> Linux. The other network interfaces can see a 'link up', but are
>> unable to transmit data. The reports were generally made on the Armada
>> 370-based Mirabox, but have also been given on the Armada 370-RD
>> board.
...
>
> David,
>
> Offending patch is:
>
> c5aff18 net: mvneta: driver for Marvell Armada 370/XP network unit
>
> Applies and builds cleanly against v3.8.13, v3.9.11, v3.10.10, and v3.11
>
> Acked-by: Jason Cooper <jason@lakedaemon.net>
Applied.
^ permalink raw reply
* Re: [PATCH net] net: netlink: filter particular protocols from analyzers
From: David Miller @ 2013-09-05 18:44 UTC (permalink / raw)
To: dborkman; +Cc: netdev, dborkmann
In-Reply-To: <1378396127-8342-1-git-send-email-dborkman@redhat.com>
From: Daniel Borkmann <dborkman@redhat.com>
Date: Thu, 5 Sep 2013 17:48:47 +0200
> From: Daniel Borkmann <dborkmann@redhat.com>
>
> Fix finer-grained control and let only a whitelist of allowed netlink
> protocols pass, in our case related to networking. If later on, other
> subsystems decide they want to add their protocol as well to the list
> of allowed protocols they shall simply add it. While at it, we also
> need to tell what protocol is in use otherwise BPF_S_ANC_PROTOCOL can
> not pick it up (as it's not filled out).
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
This takes away functionality that I'd be more interesting in using,
namely being able to listen to all netlink protocols using one tap.
Seriously, when I first saw this feature, that was the first way I'd
imagine myself using it, as a tcpdump for netlink traffic, all of
it.
If I just want to hear all netlink traffic, don't make me be forced to
know every single NETLINK_* protocol value and have to open that many
sockets just to do so.
It also makes it so that I can't listen to userlevel custom netlink
protocols, another minus of filtering.
At the very least, allow an sk_protocol of zero or similar to have this
meaning of "everything".
I'm not applying this patch, sorry.
^ permalink raw reply
* Re: [PATCH] openvswitch: Fix alignment of struct sw_flow_key.
From: David Miller @ 2013-09-05 18:40 UTC (permalink / raw)
To: jesse-l0M0P4e3n4LQT0dZR+AlfA
Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA,
fengguan.wu-ral2JQCrhuEAvxtiuMwx3w, geert-Td1EMuHUCqxL1ZNQvxDV9g
In-Reply-To: <CAEP_g=8gcCtkv=no=HkVmS+NDWPM-Uu-SVyR+vb-YQ=7TWJsXA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
From: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
Date: Thu, 5 Sep 2013 11:36:19 -0700
> On Thu, Sep 5, 2013 at 11:17 AM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
>> From: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
>> Date: Thu, 5 Sep 2013 10:41:27 -0700
>>
>>> -} __aligned(__alignof__(long));
>>> +} __aligned(8); /* 8 byte alignment ensures this can be accessed as a long */
>>
>> This kind of stuff drives me crazy.
>>
>> If the issue is the type, therefore at least use an expression that
>> mentions the type explicitly. And mention the actual type that
>> matters. "long" isn't it.
>
> 'long' actually is the real type here.
>
> When doing comparisons, this structure is being accessed as a byte
> array in 'long' sized chunks, not by its members. Therefore, the
> compiler's alignment does not necessarily correspond to anything for
> this purpose. It could be a struct full of u16's and we would still
> want to access it in chunks of 'long'.
>
> To completely honest, I think the correct alignment should be
> sizeof(long) because I know that 'long' is not always 8 bytes on all
> architectures. However, you made the point before that this could
> break the alignment of the 64-bit values on architectures where 'long'
> is 32 bits wide, so 8 bytes is the generic solution.
Look at net/core/flow.c:flow_key_compare().
And then we annotate struct flowi with
} __attribute__((__aligned__(BITS_PER_LONG/8)));
Don't reinvent the wheel, either mimick how existing code does
this kind of thing or provide a justification for doing it differently
and update the existing cases to match and be consistent.
^ permalink raw reply
* Re: linux-next: Tree for Sep 5 (netfilter: xt_socket.c)
From: David Miller @ 2013-09-05 18:38 UTC (permalink / raw)
To: rdunlap; +Cc: sfr, linux-next, linux-kernel, netdev, netfilter-devel
In-Reply-To: <5228C140.1010504@infradead.org>
From: Randy Dunlap <rdunlap@infradead.org>
Date: Thu, 05 Sep 2013 10:37:04 -0700
> On 09/05/13 02:32, Stephen Rothwell wrote:
>> Hi all,
>>
>> Please do not add any code for v3.13 to your linux-next included branches
>> until after v3.12-rc1 is released.
>>
>> Changes since 20130904:
>>
>
> on x86_64:
>
> when CONFIG_IPV6=m
> and CONFIG_NETFILTER_XT_MATCH_SOCKET=y:
>
> net/built-in.o: In function `socket_mt6_v1_v2':
> xt_socket.c:(.text+0x51b55): undefined reference to `udp6_lib_lookup'
> net/built-in.o: In function `socket_mt_init':
> xt_socket.c:(.init.text+0x1ef8): undefined reference to `nf_defrag_ipv6_enable'
I just commited the following to fix this:
--------------------
[PATCH] netfilter: Fix build errors with xt_socket.c
As reported by Randy Dunlap:
====================
when CONFIG_IPV6=m
and CONFIG_NETFILTER_XT_MATCH_SOCKET=y:
net/built-in.o: In function `socket_mt6_v1_v2':
xt_socket.c:(.text+0x51b55): undefined reference to `udp6_lib_lookup'
net/built-in.o: In function `socket_mt_init':
xt_socket.c:(.init.text+0x1ef8): undefined reference to `nf_defrag_ipv6_enable'
====================
Like several other modules under net/netfilter/ we have to
have a dependency "IPV6 disabled or set compatibly with this
module" clause.
Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
net/netfilter/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
index 62a171a..6e839b6 100644
--- a/net/netfilter/Kconfig
+++ b/net/netfilter/Kconfig
@@ -1175,6 +1175,7 @@ config NETFILTER_XT_MATCH_SOCKET
depends on NETFILTER_XTABLES
depends on NETFILTER_ADVANCED
depends on !NF_CONNTRACK || NF_CONNTRACK
+ depends on (IPV6 || IPV6=n)
select NF_DEFRAG_IPV4
select NF_DEFRAG_IPV6 if IP6_NF_IPTABLES
help
--
1.7.11.7
^ permalink raw reply related
* Re: [PATCH] openvswitch: Fix alignment of struct sw_flow_key.
From: Jesse Gross @ 2013-09-05 18:36 UTC (permalink / raw)
To: David Miller
Cc: netdev, dev@openvswitch.org, Andy Zhou, fengguan.wu,
Geert Uytterhoeven
In-Reply-To: <20130905.141751.945639989861997124.davem@davemloft.net>
On Thu, Sep 5, 2013 at 11:17 AM, David Miller <davem@davemloft.net> wrote:
> From: Jesse Gross <jesse@nicira.com>
> Date: Thu, 5 Sep 2013 10:41:27 -0700
>
>> -} __aligned(__alignof__(long));
>> +} __aligned(8); /* 8 byte alignment ensures this can be accessed as a long */
>
> This kind of stuff drives me crazy.
>
> If the issue is the type, therefore at least use an expression that
> mentions the type explicitly. And mention the actual type that
> matters. "long" isn't it.
'long' actually is the real type here.
When doing comparisons, this structure is being accessed as a byte
array in 'long' sized chunks, not by its members. Therefore, the
compiler's alignment does not necessarily correspond to anything for
this purpose. It could be a struct full of u16's and we would still
want to access it in chunks of 'long'.
To completely honest, I think the correct alignment should be
sizeof(long) because I know that 'long' is not always 8 bytes on all
architectures. However, you made the point before that this could
break the alignment of the 64-bit values on architectures where 'long'
is 32 bits wide, so 8 bytes is the generic solution.
^ permalink raw reply
* Re: [E1000-devel] [net-next v4 7/8] i40e: sysfs and debugfs interfaces
From: Stephen Hemminger @ 2013-09-05 18:32 UTC (permalink / raw)
To: Nelson, Shannon
Cc: Kirsher, Jeffrey T, e1000-devel@lists.sourceforge.net,
netdev@vger.kernel.org, Brandeburg, Jesse, gospo@redhat.com,
davem@davemloft.net, sassmann@redhat.com
In-Reply-To: <FC41C24E35F18A40888AACA1A36F3E416C6249AD@FMSMSX102.amr.corp.intel.com>
On Thu, 5 Sep 2013 17:53:38 +0000
"Nelson, Shannon" <shannon.nelson@intel.com> wrote:
> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Wednesday, September 04, 2013 5:38 PM
>
> [...]
>
> > Also, anything in sysfs is device specific and you really need to make
> > a strong case for why your device is special and needs an exception.
> > Other devices will have hardware switches and doing something through
> > sysfs is going to create a pain for any controller application.
> >
> > I vote against including the sysfs VEB stuff because it will become
> > a lifetime ABI.
>
> If we simply remove the VEB attributes (cvlan, mode, seid, svlan) but keep the model structure and the VSI attributes, will that satisfy your vote, or are you suggesting that we should drop the whole sysfs model that we implemented?
>
> At this point after some discussion internally, we think it would be better to simply remove the whole sysfs module - it is an optional section with no direct operational requirement. The intent was to get something started that looked useful, but perhaps we were a little premature in presenting this model for these new switch offload capabilities.
>
> As Dave rightly pointed out, we may want to bring this back in the future, but I think that we have more work to do with the community in proposing and designing a switching model, if it really is even needed. That's not an over-night thing, and we shouldn't be trying to patch up something that has fundamental concerns. Also, bringing it back later when it isn't obscured by the rest of the driver might be more successful in getting community input.
>
> sln
>
>
IMHO attributes are a nice way of handling the VSI attributes since they
seem hardware specific. Not sure how to do the right thing with switching.
Should it look like Macvlan, bridge, VXLAN, or something else.
^ permalink raw reply
* Re: Add missing braces in bnx2x:bnx2x_link_initialize
From: Dave Jones @ 2013-09-05 18:27 UTC (permalink / raw)
To: David Miller; +Cc: netdev, eilong
In-Reply-To: <20130905.142117.1916319107625842387.davem@davemloft.net>
On Thu, Sep 05, 2013 at 02:21:17PM -0400, David Miller wrote:
>
> David, I'm going to apply your missing braces patches, but I'm really irritated
> that I've told you at least 5 times to put proper subsystem prefixes into
> your subject lines.
>
> For this I'd use "bnx2x: ", for the TCP patch I'd use "tcp: " and for the
> CAIF change I'd use "caif: "
I knew I'd forget something..
> Please get into the habit of doing this, or I'm going to push back on you
> to correct this instead of fixing it up automatically for you every time,
> since the latter really is not scalable.
Sure thing.
thanks,
Dave
^ permalink raw reply
* Re: [PATCH] icplus: Use netif_running to determine device state
From: David Miller @ 2013-09-05 18:27 UTC (permalink / raw)
To: jdmason; +Cc: netdev, romieu, sorbica
In-Reply-To: <1378319161-18850-1-git-send-email-jdmason@kudzu.us>
From: Jon Mason <jdmason@kudzu.us>
Date: Wed, 4 Sep 2013 11:26:01 -0700
> Remove the __LINK_STATE_START check to verify the device is running, in
> favor of netif_running(). netif_running() performs the same check of
> __LINK_STATE_START, so the code should behave the same.
>
> Signed-off-by: Jon Mason <jdmason@kudzu.us>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] ethernet/arc/arc_emac: Fix huge delays in large file copies
From: David Miller @ 2013-09-05 18:25 UTC (permalink / raw)
To: Vineet.Gupta1; +Cc: netdev, Alexey.Brodkin, linux-kernel, arc-linux-dev
In-Reply-To: <1378295235-18928-1-git-send-email-vgupta@synopsys.com>
From: Vineet Gupta <Vineet.Gupta1@synopsys.com>
Date: Wed, 4 Sep 2013 17:17:15 +0530
> copying large files to a NFS mounted host was taking absurdly large
> time.
>
> Turns out that TX BD reclaim had a sublte bug.
>
> Loop starts off from @txbd_dirty cursor and stops when it hits a BD
> still in use by controller. However when it stops it needs to keep the
> cursor at that very BD to resume scanning in next iteration. However it
> was erroneously incrementing the cursor, causing the next scan(s) to
> fail too, unless the BD chain was completely drained out.
>
> [ARCLinux]$ ls -l -sh /disk/log.txt
> 17976 -rw-r--r-- 1 root root 17.5M Sep /disk/log.txt
>
> ========== Before =====================
> [ARCLinux]$ time cp /disk/log.txt /mnt/.
> real 31m 7.95s
> user 0m 0.00s
> sys 0m 0.10s
>
> ========== After =====================
> [ARCLinux]$ time cp /disk/log.txt /mnt/.
> real 0m 24.33s
> user 0m 0.00s
> sys 0m 0.19s
>
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
Applied.
^ permalink raw reply
* Re: [PATCH] ethernet/arc/arc_emac: optimize the Tx/Tx-reclaim paths a bit
From: David Miller @ 2013-09-05 18:24 UTC (permalink / raw)
To: Vineet.Gupta1; +Cc: netdev, Alexey.Brodkin, romieu, linux-kernel, arc-linux-dev
In-Reply-To: <1378299791-24598-1-git-send-email-vgupta@synopsys.com>
From: Vineet Gupta <Vineet.Gupta1@synopsys.com>
Date: Wed, 4 Sep 2013 18:33:11 +0530
> This came out of staring at code due to recent performance fix.
>
> * TX BD reclaim can call netif_wake_queue() once, outside the loop if
> one/more BDs were freed, NO need to do this each iteration.
>
> * TX need not look at next BD to stop the netif queue. It rather be done
> in the next tx call, when it actually fails as the queue seldom gets
> full but the check nevertheless needs to be done for each packet Tx.
> Profiled this under heavy traffic (big tar file cp, LMBench betworking
> tests) and saw not a single hit to that code.
>
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
You should keep the check in the transmit queueing code as a BUG check,
almost every driver has code of the form (using NIU as an example):
if (niu_tx_avail(rp) <= (skb_shinfo(skb)->nr_frags + 1)) {
netif_tx_stop_queue(txq);
dev_err(np->device, "%s: BUG! Tx ring full when queue awake!\n", dev->name);
rp->tx_errors++;
return NETDEV_TX_BUSY;
}
and arc_emac should too.
Otherwise queue management bugs are incredibly hard to diagnose.
I'm not applying this patch.
^ permalink raw reply
* Re: Add missing braces in bnx2x:bnx2x_link_initialize
From: David Miller @ 2013-09-05 18:21 UTC (permalink / raw)
To: davej; +Cc: netdev, eilong
In-Reply-To: <20130905034658.GA12717@redhat.com>
David, I'm going to apply your missing braces patches, but I'm really irritated
that I've told you at least 5 times to put proper subsystem prefixes into
your subject lines.
For this I'd use "bnx2x: ", for the TCP patch I'd use "tcp: " and for the
CAIF change I'd use "caif: "
Please get into the habit of doing this, or I'm going to push back on you
to correct this instead of fixing it up automatically for you every time,
since the latter really is not scalable.
^ permalink raw reply
* Re: [PATCH] Add missing braces to do_tcp_setsockopt
From: David Miller @ 2013-09-05 18:18 UTC (permalink / raw)
To: davej; +Cc: ncardwell, netdev, ycheng
In-Reply-To: <20130905175847.GB5682@redhat.com>
From: Dave Jones <davej@redhat.com>
Date: Thu, 5 Sep 2013 13:58:47 -0400
> On Thu, Sep 05, 2013 at 01:55:48PM -0400, Neal Cardwell wrote:
> > On Thu, Sep 5, 2013 at 1:43 PM, Dave Jones <davej@redhat.com> wrote:
> > > Signed-off-by: Dave Jones <davej@fedoraproject.org>
> >
> > Acked-by: Neal Cardwell <ncardwell@google.com>
> >
> > neal
>
> Crap, I forgot the
>
> introduced in eed530b6c67624db3f2cf477bac7c4d005d8f7ba ("tcp: early retransmit")
>
> Dave, shall I resend ?
I'll take care of it.
^ permalink raw reply
* Re: [PATCH] openvswitch: Fix alignment of struct sw_flow_key.
From: David Miller @ 2013-09-05 18:17 UTC (permalink / raw)
To: jesse; +Cc: netdev, dev, azhou, fengguan.wu, geert
In-Reply-To: <1378402887-17331-1-git-send-email-jesse@nicira.com>
From: Jesse Gross <jesse@nicira.com>
Date: Thu, 5 Sep 2013 10:41:27 -0700
> -} __aligned(__alignof__(long));
> +} __aligned(8); /* 8 byte alignment ensures this can be accessed as a long */
This kind of stuff drives me crazy.
If the issue is the type, therefore at least use an expression that
mentions the type explicitly. And mention the actual type that
matters. "long" isn't it.
These half-hearted attempts to fix this problem are just papering
around the issue and do not address the real issue directly.
I don't want to apply changes like this.
You don't want "8 byte alignment", you want the alignment necessary
for the types contained in the structure to be accessed without
generating exceptions. You might also want the "quickest" alignment,
and there are ways to express that as well.
This structure doesn't even contain a "long". "long" isn't even
64-bit on some architectures, and you certainly have 64-bit types in
this thing (__be64).
Let's start from the beginning, what are you actually trying to
accomplish here that isn't handled automatically by the compiler
already?
Is it "proper" alignment? That is handled automatically by the
compiler. Is it "fast" alignment? In what context and in what ways
is that important in this specific case?
^ 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