* Re: [PATCH net-next v2 4/5] selinux: bpf: Add selinux check for eBPF syscall operations
From: Chenbo Feng @ 2017-10-10 17:54 UTC (permalink / raw)
To: Stephen Smalley
Cc: Chenbo Feng, linux-security-module, netdev, SELinux,
Alexei Starovoitov, Daniel Borkmann, Lorenzo Colitti
In-Reply-To: <1507647148.30616.14.camel@tycho.nsa.gov>
On Tue, Oct 10, 2017 at 7:52 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On Tue, 2017-10-10 at 10:18 -0400, Stephen Smalley wrote:
>> On Mon, 2017-10-09 at 15:20 -0700, Chenbo Feng wrote:
>> > From: Chenbo Feng <fengc@google.com>
>> >
>> > Implement the actual checks introduced to eBPF related syscalls.
>> > This
>> > implementation use the security field inside bpf object to store a
>> > sid that
>> > identify the bpf object. And when processes try to access the
>> > object,
>> > selinux will check if processes have the right privileges. The
>> > creation
>> > of eBPF object are also checked at the general bpf check hook and
>> > new
>> > cmd introduced to eBPF domain can also be checked there.
>> >
>> > Signed-off-by: Chenbo Feng <fengc@google.com>
>> > Acked-by: Alexei Starovoitov <ast@kernel.org>
>> > ---
>> > security/selinux/hooks.c | 111
>> > ++++++++++++++++++++++++++++++++++++
>> > security/selinux/include/classmap.h | 2 +
>> > security/selinux/include/objsec.h | 4 ++
>> > 3 files changed, 117 insertions(+)
>> >
>> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> > index f5d304736852..41aba4e3d57c 100644
>> > --- a/security/selinux/hooks.c
>> > +++ b/security/selinux/hooks.c
>> > @@ -85,6 +85,7 @@
>> > #include <linux/export.h>
>> > #include <linux/msg.h>
>> > #include <linux/shm.h>
>> > +#include <linux/bpf.h>
>> >
>> > #include "avc.h"
>> > #include "objsec.h"
>> > @@ -6252,6 +6253,106 @@ static void selinux_ib_free_security(void
>> > *ib_sec)
>> > }
>> > #endif
>> >
>> > +#ifdef CONFIG_BPF_SYSCALL
>> > +static int selinux_bpf(int cmd, union bpf_attr *attr,
>> > + unsigned int size)
>> > +{
>> > + u32 sid = current_sid();
>> > + int ret;
>> > +
>> > + switch (cmd) {
>> > + case BPF_MAP_CREATE:
>> > + ret = avc_has_perm(sid, sid, SECCLASS_BPF_MAP,
>> > BPF_MAP__CREATE,
>> > + NULL);
>> > + break;
>> > + case BPF_PROG_LOAD:
>> > + ret = avc_has_perm(sid, sid, SECCLASS_BPF_PROG,
>> > BPF_PROG__LOAD,
>> > + NULL);
>> > + break;
>> > + default:
>> > + ret = 0;
>> > + break;
>> > + }
>> > +
>> > + return ret;
>> > +}
>> > +
>> > +static u32 bpf_map_fmode_to_av(fmode_t fmode)
>> > +{
>> > + u32 av = 0;
>> > +
>> > + if (f_mode & FMODE_READ)
>> > + av |= BPF_MAP__READ;
>> > + if (f_mode & FMODE_WRITE)
>> > + av |= BPF_MAP__WRITE;
>> > + return av;
>> > +}
>> > +
>> > +static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
>> > +{
>> > + u32 sid = current_sid();
>> > + struct bpf_security_struct *bpfsec;
>> > +
>> > + bpfsec = map->security;
>> > + return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_MAP,
>> > + bpf_map_fmode_to_av(fmode), NULL);
>> > +}
>> > +
>> > +static int selinux_bpf_prog(struct bpf_prog *prog)
>> > +{
>> > + u32 sid = current_sid();
>> > + struct bpf_security_struct *bpfsec;
>> > +
>> > + bpfsec = prog->aux->security;
>> > + return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_PROG,
>> > + BPF_PROG__USE, NULL);
>> > +}
>> > +
>> > +static int selinux_bpf_map_alloc(struct bpf_map *map)
>> > +{
>> > + struct bpf_security_struct *bpfsec;
>> > +
>> > + bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL);
>> > + if (!bpfsec)
>> > + return -ENOMEM;
>> > +
>> > + bpfsec->sid = current_sid();
>> > + map->security = bpfsec;
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static void selinux_bpf_map_free(struct bpf_map *map)
>> > +{
>> > + struct bpf_security_struct *bpfsec = map->security;
>> > +
>> > + map->security = NULL;
>> > + kfree(bpfsec);
>> > +}
>> > +
>> > +static int selinux_bpf_prog_alloc(struct bpf_prog_aux *aux)
>> > +{
>> > + struct bpf_security_struct *bpfsec;
>> > +
>> > + bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL);
>> > + if (!bpfsec)
>> > + return -ENOMEM;
>> > +
>> > + bpfsec->sid = current_sid();
>> > + aux->security = bpfsec;
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
>> > +{
>> > + struct bpf_security_struct *bpfsec = aux->security;
>> > +
>> > + aux->security = NULL;
>> > + kfree(bpfsec);
>> > +}
>> > +#endif
>> > +
>> > static struct security_hook_list selinux_hooks[]
>> > __lsm_ro_after_init
>> > = {
>> > LSM_HOOK_INIT(binder_set_context_mgr,
>> > selinux_binder_set_context_mgr),
>> > LSM_HOOK_INIT(binder_transaction,
>> > selinux_binder_transaction),
>> > @@ -6471,6 +6572,16 @@ static struct security_hook_list
>> > selinux_hooks[] __lsm_ro_after_init = {
>> > LSM_HOOK_INIT(audit_rule_match, selinux_audit_rule_match),
>> > LSM_HOOK_INIT(audit_rule_free, selinux_audit_rule_free),
>> > #endif
>> > +
>> > +#ifdef CONFIG_BPF_SYSCALL
>> > + LSM_HOOK_INIT(bpf, selinux_bpf),
>> > + LSM_HOOK_INIT(bpf_map, selinux_bpf_map),
>> > + LSM_HOOK_INIT(bpf_prog, selinux_bpf_prog),
>> > + LSM_HOOK_INIT(bpf_map_alloc_security,
>> > selinux_bpf_map_alloc),
>> > + LSM_HOOK_INIT(bpf_prog_alloc_security,
>> > selinux_bpf_prog_alloc),
>> > + LSM_HOOK_INIT(bpf_map_free_security,
>> > selinux_bpf_map_free),
>> > + LSM_HOOK_INIT(bpf_prog_free_security,
>> > selinux_bpf_prog_free),
>> > +#endif
>> > };
>> >
>> > static __init int selinux_init(void)
>> > diff --git a/security/selinux/include/classmap.h
>> > b/security/selinux/include/classmap.h
>> > index 35ffb29a69cb..7253c5eea59c 100644
>> > --- a/security/selinux/include/classmap.h
>> > +++ b/security/selinux/include/classmap.h
>> > @@ -237,6 +237,8 @@ struct security_class_mapping secclass_map[] =
>> > {
>> > { "access", NULL } },
>> > { "infiniband_endport",
>> > { "manage_subnet", NULL } },
>> > + { "bpf_map", {"create", "read", "write"} },
>> > + { "bpf_prog", {"load", "use"} },
>>
>> Again I have to ask: do you truly need/want two separate classes, or
>> would a single class with distinct permissions suffice, ala:
>> { "bpf", { "create_map", "read_map", "write_map",
>> "prog_load",
>> "prog_use" } },
>>
>> and then allow A self:bpf { create_map read_map write_map prog_load
>> prog_use }; would be stored in a single policy avtab rule, and be
>> cached in a single AVC entry.
>
Sorry I missed to reply on this, I keep it that way because sometimes
we need to grant the permission of accessing eBPF maps and programs
separately. But keep them in a single class definitely works for me.
> I guess for consistency in naming it should be either:
> { "bpf", { "map_create", "map_read", "map_write", "prog_load",
> "prog_use" } },
>
> or:
>
> { "bpf", { "create_map", "read_map", "write_map", "load_prog",
> "use_prog" } },
>
>
>> > { NULL }
>> > };
>> >
>> > diff --git a/security/selinux/include/objsec.h
>> > b/security/selinux/include/objsec.h
>> > index 1649cd18eb0b..3d54468ce334 100644
>> > --- a/security/selinux/include/objsec.h
>> > +++ b/security/selinux/include/objsec.h
>> > @@ -150,6 +150,10 @@ struct pkey_security_struct {
>> > u32 sid; /* SID of pkey */
>> > };
>> >
>> > +struct bpf_security_struct {
>> > + u32 sid; /*SID of bpf obj creater*/
>> > +};
>> > +
>> > extern unsigned int selinux_checkreqprot;
>> >
>> > #endif /* _SELINUX_OBJSEC_H_ */
^ permalink raw reply
* [PATCH][net-next] ipv6: fix incorrect bitwise operator used on rt6i_flags
From: Colin King @ 2017-10-10 17:55 UTC (permalink / raw)
To: David S . Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The use of the | operator always leads to true on the expression
(rt->rt6i_flags | RTF_CACHE) which looks rather suspect to me. I
believe this is fixed by using & instead to just check the
RTF_CACHE entry bit.
Detected by CoverityScan, CID#1457747 ("Wrong operator used")
Fixes: 35732d01fe31 ("ipv6: introduce a hash table to store dst cache")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
net/ipv6/route.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 6db1541eaa7b..0556d1ee189c 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1425,7 +1425,7 @@ int rt6_remove_exception_rt(struct rt6_info *rt)
int err;
if (!from ||
- !(rt->rt6i_flags | RTF_CACHE))
+ !(rt->rt6i_flags & RTF_CACHE))
return -EINVAL;
if (!rcu_access_pointer(from->rt6i_exception_bucket))
--
2.14.1
^ permalink raw reply related
* Re: [PATCH net-next v2 4/5] selinux: bpf: Add selinux check for eBPF syscall operations
From: kbuild test robot @ 2017-10-10 17:59 UTC (permalink / raw)
To: Chenbo Feng
Cc: kbuild-all, linux-security-module, netdev, SELinux,
Jeffrey Vander Stoep, Alexei Starovoitov, lorenzo,
Daniel Borkmann, Stephen Smalley, Chenbo Feng
In-Reply-To: <20171009222028.13096-5-chenbofeng.kernel@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1370 bytes --]
Hi Chenbo,
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Chenbo-Feng/bpf-security-New-file-mode-and-LSM-hooks-for-eBPF-object-permission-control/20171011-010349
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa
All errors (new ones prefixed by >>):
security//selinux/hooks.c: In function 'bpf_map_fmode_to_av':
>> security//selinux/hooks.c:6284:6: error: 'f_mode' undeclared (first use in this function)
if (f_mode & FMODE_READ)
^
security//selinux/hooks.c:6284:6: note: each undeclared identifier is reported only once for each function it appears in
vim +/f_mode +6284 security//selinux/hooks.c
6279
6280 static u32 bpf_map_fmode_to_av(fmode_t fmode)
6281 {
6282 u32 av = 0;
6283
> 6284 if (f_mode & FMODE_READ)
6285 av |= BPF_MAP__READ;
6286 if (f_mode & FMODE_WRITE)
6287 av |= BPF_MAP__WRITE;
6288 return av;
6289 }
6290
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51658 bytes --]
^ permalink raw reply
* NACK: [PATCH][net-next] ipv6: fix incorrect bitwise operator used on rt6i_flags
From: Colin Ian King @ 2017-10-10 18:05 UTC (permalink / raw)
To: David S . Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev
Cc: kernel-janitors, linux-kernel
In-Reply-To: <20171010175527.21982-1-colin.king@canonical.com>
On 10/10/17 18:55, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The use of the | operator always leads to true on the expression
> (rt->rt6i_flags | RTF_CACHE) which looks rather suspect to me. I
> believe this is fixed by using & instead to just check the
> RTF_CACHE entry bit.
>
> Detected by CoverityScan, CID#1457747 ("Wrong operator used")
>
> Fixes: 35732d01fe31 ("ipv6: introduce a hash table to store dst cache")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> net/ipv6/route.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 6db1541eaa7b..0556d1ee189c 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -1425,7 +1425,7 @@ int rt6_remove_exception_rt(struct rt6_info *rt)
> int err;
>
> if (!from ||
> - !(rt->rt6i_flags | RTF_CACHE))
> + !(rt->rt6i_flags & RTF_CACHE))
> return -EINVAL;
>
> if (!rcu_access_pointer(from->rt6i_exception_bucket))
>
Nack that, seems like this occurs more than once and I failed to spot
the others.
^ permalink raw reply
* Re: [RFC net 1/1] net: sched: act: fix rcu race in dump
From: Alexander Aring @ 2017-10-10 18:09 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jamal Hadi Salim, Cong Wang, Jiří Pírko, netdev,
Manish Kurup, Brenda Butler
In-Reply-To: <1507644745.31614.17.camel@edumazet-glaptop3.roam.corp.google.com>
Hi,
On Tue, Oct 10, 2017 at 10:12 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2017-10-10 at 08:32 -0400, Alexander Aring wrote:
>> This patch fixes an issue with kfree_rcu which is not protected by RTNL
>> lock. It could be that the current assigned rcu pointer will be freed by
>> kfree_rcu while dump callback is running.
>>
>> To prevent this, we call rcu_synchronize at first. Then we are sure all
>> latest rcu functions e.g. rcu_assign_pointer and kfree_rcu in init are
>> done. After rcu_synchronize we dereference under RTNL lock which is also
>> held in init function, which means no other rcu_assign_pointer or
>> kfree_rcu will occur.
>>
>> To call rcu_synchronize will also prevent weird behaviours by doing over
>> netlink:
>>
>> - set params A
>> - set params B
>> - dump params
>> \--> will dump params A
>>
>> This could be a unlikely case that the last rcu_assign_pointer was not
>> happened before dump callback.
>>
>> Signed-off-by: Alexander Aring <aring@mojatatu.com>
>> ---
>> net/sched/act_skbmod.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
>> index b642ad3d39dd..231e07bca384 100644
>> --- a/net/sched/act_skbmod.c
>> +++ b/net/sched/act_skbmod.c
>> @@ -198,7 +198,7 @@ static int tcf_skbmod_dump(struct sk_buff *skb, struct tc_action *a,
>> {
>> struct tcf_skbmod *d = to_skbmod(a);
>> unsigned char *b = skb_tail_pointer(skb);
>> - struct tcf_skbmod_params *p = rtnl_dereference(d->skbmod_p);
>> + struct tcf_skbmod_params *p;
>> struct tc_skbmod opt = {
>> .index = d->tcf_index,
>> .refcnt = d->tcf_refcnt - ref,
>> @@ -207,6 +207,11 @@ static int tcf_skbmod_dump(struct sk_buff *skb, struct tc_action *a,
>> };
>> struct tcf_t t;
>>
>> + /* wait until last rcu_assign_pointer/kfree_rcu is done */
>> + rcu_synchronize();
>> + /* RTNL lock prevents another rcu_assign_pointer/kfree_rcu call */
>> + p = rtnl_dereference(d->skbmod_p);
>> +
>> opt.flags = p->flags;
>> if (nla_put(skb, TCA_SKBMOD_PARMS, sizeof(opt), &opt))
>> goto nla_put_failure;
>
> Sorry but no. This is plainly wrong.
>
> We need to fix this without adding a _very_ expensive rcu_synchronize()
> on a path which does not need such thing.
>
I agree that a rcu synchronize is very expensive while holding RTNL.
Should be handled with rcu_read_lock as you suggested below, but this
will not prevent to show an user space behavior like:
- set_params(A)
- set_params(B)
\---> dump - will dump values A
Because the rcu_read_lock will avoid rcu_assign_pointer to update the
pointer and not wait that the rcu_assign_pointer of set_params(B) is
done before calling dump.
Okay, this issue is maybe something we should not care about it so far
it's not an use after free issue.
> I am confused by this patch, please tell us more what the problem is.
>
The callback "init" is also called by updating parameters for an action.
It use rcu_assign_pointer [0], as well kfree_rcu [1] to swap the
pointers of parameter structures and free the old resource.
This is well protected by rcu_read_lock inside the "run" callback of
tc action, which runs in softirq context. But dump is only protected
by RTNL so far I see.
Sorry when I understood RCU wrong, but so far I understood RCU
handling, it _could_ be that returning of "init" the pointers are not
updated yet. After a "grace" period, which rcu synchronize waits for
it - we can be sure that it's assigned and kfree_rcu completes.
The problem is:
If the deference of parameters inside dump callback using still the
old structure (for my understanding, it can happened because this
callback do nothing against it to protect it) kfree_rcu can free the
resource during accessing this structure. A RCU read lock will of
course preventing RCU to update the pointers in this time (but not
RTNL, so far I understood).
> I suspect rcu_read_lock() is what you need, but isn't a writer supposed
> to hold RTNL in net/sched/* ???
>
Yes a writer holds RTNL, but these writers using RCU to write (as
shown in [0] and [1]). So far I know kfree_rcu: it can occur that
"init" returns and dump is called afterwards - during the dump RCU can
run and free/assign pointers in this time (while dump still holds
references). So far I understand a RTNL lock will not prevent RCU to
do that.
I wrote this mail also to get an answer if there exists a problem or
not. If you say me, the resource cannot be freed by kfree_rcu if RTNL
lock is hold, then I know more about how RCU is working now.
- Alex
[0] http://elixir.free-electrons.com/linux/latest/source/net/sched/act_skbmod.c#L177
[1] http://elixir.free-electrons.com/linux/latest/source/net/sched/act_skbmod.c#L182
^ permalink raw reply
* [PATCH][V2] ipv6: fix incorrect bitwise operator used on rt6i_flags
From: Colin King @ 2017-10-10 18:10 UTC (permalink / raw)
To: David S . Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The use of the | operator always leads to true which looks rather
suspect to me. Fix this by using & instead to just check the
RTF_CACHE entry bit.
Detected by CoverityScan, CID#1457734, #1457747 ("Wrong operator used")
Fixes: 35732d01fe31 ("ipv6: introduce a hash table to store dst cache")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
net/ipv6/route.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 6db1541eaa7b..dd9ba1192dbc 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1425,7 +1425,7 @@ int rt6_remove_exception_rt(struct rt6_info *rt)
int err;
if (!from ||
- !(rt->rt6i_flags | RTF_CACHE))
+ !(rt->rt6i_flags & RTF_CACHE))
return -EINVAL;
if (!rcu_access_pointer(from->rt6i_exception_bucket))
@@ -1469,7 +1469,7 @@ static void rt6_update_exception_stamp_rt(struct rt6_info *rt)
struct rt6_exception *rt6_ex;
if (!from ||
- !(rt->rt6i_flags | RTF_CACHE))
+ !(rt->rt6i_flags & RTF_CACHE))
return;
rcu_read_lock();
--
2.14.1
^ permalink raw reply related
* Re: [PATCH][net-next] ipv6: fix incorrect bitwise operator used on rt6i_flags
From: Martin KaFai Lau @ 2017-10-10 18:10 UTC (permalink / raw)
To: Colin King
Cc: David S . Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev,
kernel-janitors, linux-kernel, Wei Wang
In-Reply-To: <20171010175527.21982-1-colin.king@canonical.com>
On Tue, Oct 10, 2017 at 05:55:27PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The use of the | operator always leads to true on the expression
> (rt->rt6i_flags | RTF_CACHE) which looks rather suspect to me. I
> believe this is fixed by using & instead to just check the
> RTF_CACHE entry bit.
Good catch. LGTM. If rt does not have RTF_CACHE set, it should not be in the
exception table.
Acked-by: Martin KaFai Lau <kafai@fb.com>
>
> Detected by CoverityScan, CID#1457747 ("Wrong operator used")
>
> Fixes: 35732d01fe31 ("ipv6: introduce a hash table to store dst cache")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> net/ipv6/route.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 6db1541eaa7b..0556d1ee189c 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -1425,7 +1425,7 @@ int rt6_remove_exception_rt(struct rt6_info *rt)
> int err;
>
> if (!from ||
> - !(rt->rt6i_flags | RTF_CACHE))
> + !(rt->rt6i_flags & RTF_CACHE))
> return -EINVAL;
>
> if (!rcu_access_pointer(from->rt6i_exception_bucket))
> --
> 2.14.1
>
^ permalink raw reply
* Re: [PATCH][net-next] ipv6: fix incorrect bitwise operator used on rt6i_flags
From: Wei Wang @ 2017-10-10 18:23 UTC (permalink / raw)
To: Martin KaFai Lau, Colin King, Eric Dumazet
Cc: David S . Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
Linux Kernel Network Developers, kernel-janitors, lkml
In-Reply-To: <20171010181054.4snwnyc3q7rf2wbl@kafai-mbp.dhcp.thefacebook.com>
On Tue, Oct 10, 2017 at 11:10 AM, Martin KaFai Lau <kafai@fb.com> wrote:
> On Tue, Oct 10, 2017 at 05:55:27PM +0000, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The use of the | operator always leads to true on the expression
>> (rt->rt6i_flags | RTF_CACHE) which looks rather suspect to me. I
>> believe this is fixed by using & instead to just check the
>> RTF_CACHE entry bit.
> Good catch. LGTM. If rt does not have RTF_CACHE set, it should not be in the
> exception table.
>
> Acked-by: Martin KaFai Lau <kafai@fb.com>
>
Thanks a lot for catching this. Yes. It should have been '&' instead of '|'.
Acked-by: Wei Wang <weiwan@google.com>
>>
>> Detected by CoverityScan, CID#1457747 ("Wrong operator used")
>>
>> Fixes: 35732d01fe31 ("ipv6: introduce a hash table to store dst cache")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>> net/ipv6/route.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index 6db1541eaa7b..0556d1ee189c 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -1425,7 +1425,7 @@ int rt6_remove_exception_rt(struct rt6_info *rt)
>> int err;
>>
>> if (!from ||
>> - !(rt->rt6i_flags | RTF_CACHE))
>> + !(rt->rt6i_flags & RTF_CACHE))
>> return -EINVAL;
>>
>> if (!rcu_access_pointer(from->rt6i_exception_bucket))
>> --
>> 2.14.1
>>
^ permalink raw reply
* Re: [PATCH][net-next] ipv6: fix incorrect bitwise operator used on rt6i_flags
From: Colin Ian King @ 2017-10-10 18:24 UTC (permalink / raw)
To: Wei Wang, Martin KaFai Lau, Eric Dumazet
Cc: David S . Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
Linux Kernel Network Developers, kernel-janitors, lkml
In-Reply-To: <CAEA6p_BDQ_RaF7-9eBnhstMY_w-i91cJECXuvSHntkdDQo-YUw@mail.gmail.com>
On 10/10/17 19:23, Wei Wang wrote:
> On Tue, Oct 10, 2017 at 11:10 AM, Martin KaFai Lau <kafai@fb.com> wrote:
>> On Tue, Oct 10, 2017 at 05:55:27PM +0000, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> The use of the | operator always leads to true on the expression
>>> (rt->rt6i_flags | RTF_CACHE) which looks rather suspect to me. I
>>> believe this is fixed by using & instead to just check the
>>> RTF_CACHE entry bit.
>> Good catch. LGTM. If rt does not have RTF_CACHE set, it should not be in the
>> exception table.
>>
>> Acked-by: Martin KaFai Lau <kafai@fb.com>
>>
>
> Thanks a lot for catching this. Yes. It should have been '&' instead of '|'.
>
> Acked-by: Wei Wang <weiwan@google.com>
Sorry, can you look at V2 of this patch; there is one more occurrence
that needed fixing.
>
>>>
>>> Detected by CoverityScan, CID#1457747 ("Wrong operator used")
>>>
>>> Fixes: 35732d01fe31 ("ipv6: introduce a hash table to store dst cache")
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>> ---
>>> net/ipv6/route.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>> index 6db1541eaa7b..0556d1ee189c 100644
>>> --- a/net/ipv6/route.c
>>> +++ b/net/ipv6/route.c
>>> @@ -1425,7 +1425,7 @@ int rt6_remove_exception_rt(struct rt6_info *rt)
>>> int err;
>>>
>>> if (!from ||
>>> - !(rt->rt6i_flags | RTF_CACHE))
>>> + !(rt->rt6i_flags & RTF_CACHE))
>>> return -EINVAL;
>>>
>>> if (!rcu_access_pointer(from->rt6i_exception_bucket))
>>> --
>>> 2.14.1
>>>
^ permalink raw reply
* Re: [PATCH][V2] ipv6: fix incorrect bitwise operator used on rt6i_flags
From: Wei Wang @ 2017-10-10 18:38 UTC (permalink / raw)
To: Colin King
Cc: David S . Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
Linux Kernel Network Developers, kernel-janitors, linux-kernel
In-Reply-To: <20171010181030.22290-1-colin.king@canonical.com>
On Tue, Oct 10, 2017 at 11:10 AM, Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The use of the | operator always leads to true which looks rather
> suspect to me. Fix this by using & instead to just check the
> RTF_CACHE entry bit.
>
> Detected by CoverityScan, CID#1457734, #1457747 ("Wrong operator used")
>
> Fixes: 35732d01fe31 ("ipv6: introduce a hash table to store dst cache")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
Acked-by: Wei Wang <weiwan@google.com>
> net/ipv6/route.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 6db1541eaa7b..dd9ba1192dbc 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -1425,7 +1425,7 @@ int rt6_remove_exception_rt(struct rt6_info *rt)
> int err;
>
> if (!from ||
> - !(rt->rt6i_flags | RTF_CACHE))
> + !(rt->rt6i_flags & RTF_CACHE))
> return -EINVAL;
>
> if (!rcu_access_pointer(from->rt6i_exception_bucket))
> @@ -1469,7 +1469,7 @@ static void rt6_update_exception_stamp_rt(struct rt6_info *rt)
> struct rt6_exception *rt6_ex;
>
> if (!from ||
> - !(rt->rt6i_flags | RTF_CACHE))
> + !(rt->rt6i_flags & RTF_CACHE))
> return;
>
> rcu_read_lock();
> --
> 2.14.1
>
^ permalink raw reply
* RE: [PATCH v5 1/2] net: phy: DP83822 initial driver submission
From: Woojung.Huh @ 2017-10-10 18:56 UTC (permalink / raw)
To: dmurphy, andrew, f.fainelli; +Cc: netdev, afd
In-Reply-To: <20171010174256.21930-1-dmurphy@ti.com>
> +static int dp83822_config_intr(struct phy_device *phydev)
> +{
> + int misr_status;
> + int physcr_status;
> + int err;
> +
> + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> + misr_status = phy_read(phydev, MII_DP83822_MISR1);
> + if (misr_status < 0)
> + return misr_status;
> +
> + misr_status |= (DP83822_RX_ERR_HF_INT_EN |
> + DP83822_FALSE_CARRIER_HF_INT_EN |
> + DP83822_ANEG_COMPLETE_INT_EN |
> + DP83822_DUP_MODE_CHANGE_INT_EN |
> + DP83822_SPEED_CHANGED_INT_EN |
> + DP83822_LINK_STAT_INT_EN |
> + DP83822_ENERGY_DET_INT_EN |
> + DP83822_LINK_QUAL_INT_EN);
> +
> + err = phy_write(phydev, MII_DP83822_MISR1, misr_status);
> + if (err < 0)
> + return err;
> +
> + misr_status = phy_read(phydev, MII_DP83822_MISR2);
> + if (misr_status < 0)
> + return misr_status;
> +
> + misr_status |= (DP83822_JABBER_DET_INT_EN |
> + DP83822_WOL_PKT_INT_EN |
> + DP83822_SLEEP_MODE_INT_EN |
> + DP83822_MDI_XOVER_INT_EN |
> + DP83822_LB_FIFO_INT_EN |
> + DP83822_PAGE_RX_INT_EN |
> + DP83822_ANEG_ERR_INT_EN |
> + DP83822_EEE_ERROR_CHANGE_INT_EN);
> +
> + err = phy_write(phydev, MII_DP83822_MISR2, misr_status);
> + if (err < 0)
> + return err;
> +
> + physcr_status = phy_read(phydev, MII_DP83822_PHYSCR);
> + if (physcr_status < 0)
> + return physcr_status;
> +
> + physcr_status |= DP83822_PHYSCR_INT_OE |
> DP83822_PHYSCR_INTEN;
> +
Don't want to be picky, but seeing extra blank line here.
> + } else {
> + err = phy_write(phydev, MII_DP83822_MISR1, 0);
> + if (err < 0)
> + return err;
> +
> + err = phy_write(phydev, MII_DP83822_MISR1, 0);
> + if (err < 0)
> + return err;
> +
> + physcr_status = phy_read(phydev, MII_DP83822_PHYSCR);
> + if (physcr_status < 0)
> + return physcr_status;
> +
> + physcr_status &= ~DP83822_PHYSCR_INTEN;
> + }
^ permalink raw reply
* RE: [PATCH net 2/2] net/smc: dev_put for netdev after usage of ib_query_gid()
From: Parav Pandit @ 2017-10-10 19:04 UTC (permalink / raw)
To: Parav Pandit, Ursula Braun,
davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-s390-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
jwi-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org,
schwidefsky-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org,
heiko.carstens-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org,
raspl-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org
In-Reply-To: <VI1PR0502MB30088E4F58036AB3A06822E8D1750-o1MPJYiShExKsLr+rGaxW8DSnupUy6xnnBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
> -----Original Message-----
> > For ROCEs ib_query_gid() takes a reference count on the net_device.
> > This reference count must be decreased by the caller.
> >
> > Signed-off-by: Ursula Braun <ubraun-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Reported-by: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>
Also add 'Fixes' line that introduced this bug.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2] xdp: Sample xdp program implementing ip forward
From: Jesper Dangaard Brouer @ 2017-10-10 19:06 UTC (permalink / raw)
To: Christina Jacob
Cc: netdev, linux-kernel, linux-arm-kernel, Sunil.Goutham, daniel,
dsahern, Christina Jacob, brouer
In-Reply-To: <1507620532-25804-2-git-send-email-Christina.Jacob@cavium.com>
On Tue, 10 Oct 2017 12:58:52 +0530
Christina Jacob <christina.jacob.koikara@gmail.com> wrote:
> +SEC("xdp3")
> +int xdp_prog3(struct xdp_md *ctx)
> +{
> + void *data_end = (void *)(long)ctx->data_end;
> + void *data = (void *)(long)ctx->data;
> + struct ethhdr *eth = data;
> + int rc = XDP_DROP, forward_to;
> + long *value;
> + struct trie_value *prefix_value;
> + long *dest_mac = NULL, *src_mac = NULL;
> + u16 h_proto;
> + u64 nh_off;
> + u32 ipproto;
> + union key_4 key4;
Reverse-xmas tree rule: Prefer ordering declarations longest to shortest.
[...]
> + if (h_proto == htons(ETH_P_ARP)) {
> + return XDP_PASS;
> + } else if (h_proto == htons(ETH_P_IP)) {
> + int src_ip = 0, dest_ip = 0;
> + struct direct_map *direct_entry;
> +
> + ipproto = parse_ipv4(data, nh_off, data_end, &src_ip, &dest_ip);
> + direct_entry = (struct direct_map *)bpf_map_lookup_elem
> + (&exact_match, &dest_ip);
> + /*check for exact match, this would give a faster lookup*/
> + if (direct_entry && direct_entry->mac && direct_entry->arp.mac) {
> + src_mac = &direct_entry->mac;
> + dest_mac = &direct_entry->arp.mac;
> + forward_to = direct_entry->ifindex;
> + } else {
> + /*Look up in the trie for lpm*/
> + key4.b32[0] = 32;
> + key4.b8[4] = dest_ip % 0x100;
> + key4.b8[5] = (dest_ip >> 8) % 0x100;
> + key4.b8[6] = (dest_ip >> 16) % 0x100;
> + key4.b8[7] = (dest_ip >> 24) % 0x100;
> + prefix_value = ((struct trie_value *)bpf_map_lookup_elem
> + (&lpm_map, &key4));
> + if (!prefix_value) {
> + return XDP_DROP;
> + } else {
> + src_mac = &prefix_value->value;
> + if (src_mac) {
> + dest_mac = (long *)bpf_map_lookup_elem
> + (&arp_table, &dest_ip);
> + if (!dest_mac) {
> + if (prefix_value->gw) {
> + dest_ip = *(__be32 *)&prefix_value->gw;
> + dest_mac = (long *)bpf_map_lookup_elem(&arp_table, &dest_ip);
> + } else {
> + return XDP_DROP;
> + }
> + }
> + forward_to = prefix_value->ifindex;
> + } else {
> + return XDP_DROP;
> + }
> + }
> + }
> + } else {
> + ipproto = 0;
> + }
The nesting in this function is getting annoying to read.
Kernel code often use "early return" style coding to solve this.
A quick search turns up this guide, look at section "Early return"
https://en.wikibooks.org/wiki/Computer_Programming/Coding_Style/Minimize_nesting
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH v5 1/2] net: phy: DP83822 initial driver submission
From: Dan Murphy @ 2017-10-10 19:05 UTC (permalink / raw)
To: Woojung.Huh, andrew, f.fainelli; +Cc: netdev, afd
In-Reply-To: <9235D6609DB808459E95D78E17F2E43D40B48F9D@CHN-SV-EXMX02.mchp-main.com>
Florian
On 10/10/2017 01:56 PM, Woojung.Huh@microchip.com wrote:
>> +static int dp83822_config_intr(struct phy_device *phydev)
>> +{
>> + int misr_status;
>> + int physcr_status;
>> + int err;
>> +
>> + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
>> + misr_status = phy_read(phydev, MII_DP83822_MISR1);
>> + if (misr_status < 0)
>> + return misr_status;
>> +
>> + misr_status |= (DP83822_RX_ERR_HF_INT_EN |
>> + DP83822_FALSE_CARRIER_HF_INT_EN |
>> + DP83822_ANEG_COMPLETE_INT_EN |
>> + DP83822_DUP_MODE_CHANGE_INT_EN |
>> + DP83822_SPEED_CHANGED_INT_EN |
>> + DP83822_LINK_STAT_INT_EN |
>> + DP83822_ENERGY_DET_INT_EN |
>> + DP83822_LINK_QUAL_INT_EN);
>> +
>> + err = phy_write(phydev, MII_DP83822_MISR1, misr_status);
>> + if (err < 0)
>> + return err;
>> +
>> + misr_status = phy_read(phydev, MII_DP83822_MISR2);
>> + if (misr_status < 0)
>> + return misr_status;
>> +
>> + misr_status |= (DP83822_JABBER_DET_INT_EN |
>> + DP83822_WOL_PKT_INT_EN |
>> + DP83822_SLEEP_MODE_INT_EN |
>> + DP83822_MDI_XOVER_INT_EN |
>> + DP83822_LB_FIFO_INT_EN |
>> + DP83822_PAGE_RX_INT_EN |
>> + DP83822_ANEG_ERR_INT_EN |
>> + DP83822_EEE_ERROR_CHANGE_INT_EN);
>> +
>> + err = phy_write(phydev, MII_DP83822_MISR2, misr_status);
>> + if (err < 0)
>> + return err;
>> +
>> + physcr_status = phy_read(phydev, MII_DP83822_PHYSCR);
>> + if (physcr_status < 0)
>> + return physcr_status;
>> +
>> + physcr_status |= DP83822_PHYSCR_INT_OE |
>> DP83822_PHYSCR_INTEN;
>> +
> Don't want to be picky, but seeing extra blank line here.
>
Let me know if you want me to fix this up. Or if you will when you apply it.
Dan
<snip>
--
------------------
Dan Murphy
^ permalink raw reply
* Re: [PATCH RFC 0/3] tun zerocopy stats
From: Willem de Bruijn @ 2017-10-10 19:11 UTC (permalink / raw)
To: David Miller
Cc: Network Development, Michael S. Tsirkin, Jason Wang,
Willem de Bruijn
In-Reply-To: <20171010.103918.358738948563291074.davem@davemloft.net>
On Tue, Oct 10, 2017 at 1:39 PM, David Miller <davem@davemloft.net> wrote:
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Date: Tue, 10 Oct 2017 11:29:33 -0400
>
>> If there is a way to expose these stats through vhost_net directly,
>> instead of through tun, that may be better. But I did not see a
>> suitable interface. Perhaps debugfs.
>
> Please don't use debugfs, thank you :-)
Okay. I'll take a look at tracing for on-demand measurement.
^ permalink raw reply
* Re: [ovs-dev] [PATCH net-next] openvswitch: add ct_clear action
From: Eric Garver @ 2017-10-10 19:13 UTC (permalink / raw)
To: Joe Stringer; +Cc: Pravin Shelar, Linux Kernel Network Developers, ovs dev
In-Reply-To: <CAPWQB7FFNB1vghe+y_Y7B3buYmANjMTaEOgAKVHsmbOsDZxh6g@mail.gmail.com>
On Tue, Oct 10, 2017 at 10:24:20AM -0700, Joe Stringer wrote:
> On 10 October 2017 at 08:09, Eric Garver <e@erig.me> wrote:
> > On Tue, Oct 10, 2017 at 05:33:48AM -0700, Joe Stringer wrote:
> >> On 9 October 2017 at 21:41, Pravin Shelar <pshelar@ovn.org> wrote:
> >> > On Fri, Oct 6, 2017 at 9:44 AM, Eric Garver <e@erig.me> wrote:
> >> >> This adds a ct_clear action for clearing conntrack state. ct_clear is
> >> >> currently implemented in OVS userspace, but is not backed by an action
> >> >> in the kernel datapath. This is useful for flows that may modify a
> >> >> packet tuple after a ct lookup has already occurred.
> >> >>
> >> >> Signed-off-by: Eric Garver <e@erig.me>
> >> > Patch mostly looks good. I have following comments.
> >> >
> >> >> ---
> >> >> include/uapi/linux/openvswitch.h | 2 ++
> >> >> net/openvswitch/actions.c | 5 +++++
> >> >> net/openvswitch/conntrack.c | 12 ++++++++++++
> >> >> net/openvswitch/conntrack.h | 7 +++++++
> >> >> net/openvswitch/flow_netlink.c | 5 +++++
> >> >> 5 files changed, 31 insertions(+)
> >> >>
> >> >> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> >> >> index 156ee4cab82e..1b6e510e2cc6 100644
> >> >> --- a/include/uapi/linux/openvswitch.h
> >> >> +++ b/include/uapi/linux/openvswitch.h
> >> >> @@ -806,6 +806,7 @@ struct ovs_action_push_eth {
> >> >> * packet.
> >> >> * @OVS_ACTION_ATTR_POP_ETH: Pop the outermost Ethernet header off the
> >> >> * packet.
> >> >> + * @OVS_ACTION_ATTR_CT_CLEAR: Clear conntrack state from the packet.
> >> >> *
> >> >> * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all
> >> >> * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
> >> >> @@ -835,6 +836,7 @@ enum ovs_action_attr {
> >> >> OVS_ACTION_ATTR_TRUNC, /* u32 struct ovs_action_trunc. */
> >> >> OVS_ACTION_ATTR_PUSH_ETH, /* struct ovs_action_push_eth. */
> >> >> OVS_ACTION_ATTR_POP_ETH, /* No argument. */
> >> >> + OVS_ACTION_ATTR_CT_CLEAR, /* No argument. */
> >> >>
> >> >> __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted
> >> >> * from userspace. */
> >> >> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> >> >> index a54a556fcdb5..db9c7f2e662b 100644
> >> >> --- a/net/openvswitch/actions.c
> >> >> +++ b/net/openvswitch/actions.c
> >> >> @@ -1203,6 +1203,10 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> >> >> return err == -EINPROGRESS ? 0 : err;
> >> >> break;
> >> >>
> >> >> + case OVS_ACTION_ATTR_CT_CLEAR:
> >> >> + err = ovs_ct_clear(skb, key);
> >> >> + break;
> >> >> +
> >> >> case OVS_ACTION_ATTR_PUSH_ETH:
> >> >> err = push_eth(skb, key, nla_data(a));
> >> >> break;
> >> >> @@ -1210,6 +1214,7 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> >> >> case OVS_ACTION_ATTR_POP_ETH:
> >> >> err = pop_eth(skb, key);
> >> >> break;
> >> >> +
> >> >> }
> >> > Unrelated change.
> >> >
> >> >>
> >> >> if (unlikely(err)) {
> >> >> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> >> >> index d558e882ca0c..f9b73c726ad7 100644
> >> >> --- a/net/openvswitch/conntrack.c
> >> >> +++ b/net/openvswitch/conntrack.c
> >> >> @@ -1129,6 +1129,18 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
> >> >> return err;
> >> >> }
> >> >>
> >> >> +int ovs_ct_clear(struct sk_buff *skb, struct sw_flow_key *key)
> >> >> +{
> >> >> + if (skb_nfct(skb)) {
> >> >> + nf_conntrack_put(skb_nfct(skb));
> >> >> + nf_ct_set(skb, NULL, 0);
> >> > Can the new conntract state be appropriate? may be IP_CT_UNTRACKED?
> >> >
> >> >> + }
> >> >> +
> >> >> + ovs_ct_fill_key(skb, key);
> >> >> +
> >> > I do not see need to refill the key if there is no skb-nf-ct.
> >>
> >> Really this is trying to just zero the CT key fields, but reuses
> >> existing functions, right? This means that subsequent upcalls, for
> >
> > Right.
> >
> >> instance, won't have the outdated view of the CT state from the
> >> previous lookup (that was prior to the ct_clear). I'd expect these key
> >> fields to be cleared.
> >
> > I assumed Pravin was saying that we don't need to clear them if there is
> > no conntrack state. They should already be zero.
>
> The conntrack calls aren't going to clear it, so I don't see what else
> would clear it?
>
> If you execute ct(),ct_clear(), then the first ct will set the
> values.. what will zero them?
I meant move ovs_ct_fill_key() to inside the if statement.
i.e.
if (skb_nfct(skb)) {
nf_conntrack_put(skb_nfct(skb));
nf_ct_set(skb, NULL, IP_CT_UNTRACKED);
ovs_ct_fill_key(skb, key);
}
Should be nothing to fill/zero if we have not yet done conntrack.
Is there a case where we may lose skb->_nfct, but the key still has
conntrack data?
^ permalink raw reply
* Re: [PATCH][V2] ipv6: fix incorrect bitwise operator used on rt6i_flags
From: Martin KaFai Lau @ 2017-10-10 19:14 UTC (permalink / raw)
To: Colin King
Cc: David S . Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev,
kernel-janitors, linux-kernel
In-Reply-To: <20171010181030.22290-1-colin.king@canonical.com>
On Tue, Oct 10, 2017 at 06:10:30PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The use of the | operator always leads to true which looks rather
> suspect to me. Fix this by using & instead to just check the
> RTF_CACHE entry bit.
>
> Detected by CoverityScan, CID#1457734, #1457747 ("Wrong operator used")
>
> Fixes: 35732d01fe31 ("ipv6: introduce a hash table to store dst cache")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Acked-by: Martin KaFai Lau <kafai@fb.com>
> ---
> net/ipv6/route.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 6db1541eaa7b..dd9ba1192dbc 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -1425,7 +1425,7 @@ int rt6_remove_exception_rt(struct rt6_info *rt)
> int err;
>
> if (!from ||
> - !(rt->rt6i_flags | RTF_CACHE))
> + !(rt->rt6i_flags & RTF_CACHE))
> return -EINVAL;
>
> if (!rcu_access_pointer(from->rt6i_exception_bucket))
> @@ -1469,7 +1469,7 @@ static void rt6_update_exception_stamp_rt(struct rt6_info *rt)
> struct rt6_exception *rt6_ex;
>
> if (!from ||
> - !(rt->rt6i_flags | RTF_CACHE))
> + !(rt->rt6i_flags & RTF_CACHE))
> return;
>
> rcu_read_lock();
> --
> 2.14.1
>
^ permalink raw reply
* Re: [Non-DoD Source] Re: [PATCH net-next v2 5/5] selinux: bpf: Add addtional check for bpf object file receive
From: Stephen Smalley @ 2017-10-10 19:23 UTC (permalink / raw)
To: Chenbo Feng
Cc: Chenbo Feng, linux-security-module, netdev, SELinux,
Daniel Borkmann, Alexei Starovoitov, Lorenzo Colitti
In-Reply-To: <CAMOXUJnuxe8mYUv9Dw5wo40M5Bhszg+uf54dzsp6t=JiBgNZ2Q@mail.gmail.com>
On Tue, 2017-10-10 at 10:48 -0700, Chenbo Feng wrote:
> On Tue, Oct 10, 2017 at 7:24 AM, Stephen Smalley <sds@tycho.nsa.gov>
> wrote:
> > On Mon, 2017-10-09 at 15:20 -0700, Chenbo Feng wrote:
> > > From: Chenbo Feng <fengc@google.com>
> > >
> > > Introduce a bpf object related check when sending and receiving
> > > files
> > > through unix domain socket as well as binder. It checks if the
> > > receiving
> > > process have privilege to read/write the bpf map or use the bpf
> > > program.
> > > This check is necessary because the bpf maps and programs are
> > > using a
> > > anonymous inode as their shared inode so the normal way of
> > > checking
> > > the
> > > files and sockets when passing between processes cannot work
> > > properly
> > > on
> > > eBPF object. This check only works when the BPF_SYSCALL is
> > > configured.
> > > The information stored inside the file security struct is the
> > > same as
> > > the information in bpf object security struct.
> > >
> > > Signed-off-by: Chenbo Feng <fengc@google.com>
> > > ---
> > > include/linux/bpf.h | 3 +++
> > > include/linux/lsm_hooks.h | 17 +++++++++++++
> > > include/linux/security.h | 9 +++++++
> > > kernel/bpf/syscall.c | 4 ++--
> > > security/security.c | 8 +++++++
> > > security/selinux/hooks.c | 61
> > > +++++++++++++++++++++++++++++++++++++++++++++++
> > > 6 files changed, 100 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index 225740688ab7..81d6c01b8825 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -285,6 +285,9 @@ int bpf_prog_array_copy_to_user(struct
> > > bpf_prog_array __rcu *progs,
> > > #ifdef CONFIG_BPF_SYSCALL
> > > DECLARE_PER_CPU(int, bpf_prog_active);
> > >
> > > +extern const struct file_operations bpf_map_fops;
> > > +extern const struct file_operations bpf_prog_fops;
> > > +
> > > #define BPF_PROG_TYPE(_id, _ops) \
> > > extern const struct bpf_verifier_ops _ops;
> > > #define BPF_MAP_TYPE(_id, _ops) \
> > > diff --git a/include/linux/lsm_hooks.h
> > > b/include/linux/lsm_hooks.h
> > > index 7161d8e7ee79..517dea60b87b 100644
> > > --- a/include/linux/lsm_hooks.h
> > > +++ b/include/linux/lsm_hooks.h
> > > @@ -1385,6 +1385,19 @@
> > > * @bpf_prog_free_security:
> > > * Clean up the security information stored inside bpf prog.
> > > *
> > > + * @bpf_map_file:
> > > + * When creating a bpf map fd, set up the file security
> > > information with
> > > + * the bpf security information stored in the map struct. So
> > > when the map
> > > + * fd is passed between processes, the security module can
> > > directly read
> > > + * the security information from file security struct rather
> > > than the bpf
> > > + * security struct.
> > > + *
> > > + * @bpf_prog_file:
> > > + * When creating a bpf prog fd, set up the file security
> > > information with
> > > + * the bpf security information stored in the prog struct. So
> > > when the prog
> > > + * fd is passed between processes, the security module can
> > > directly read
> > > + * the security information from file security struct rather
> > > than the bpf
> > > + * security struct.
> > > */
> > > union security_list_options {
> > > int (*binder_set_context_mgr)(struct task_struct *mgr);
> > > @@ -1726,6 +1739,8 @@ union security_list_options {
> > > void (*bpf_map_free_security)(struct bpf_map *map);
> > > int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
> > > void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
> > > + void (*bpf_map_file)(struct bpf_map *map, struct file
> > > *file);
> > > + void (*bpf_prog_file)(struct bpf_prog_aux *aux, struct file
> > > *file);
> > > #endif /* CONFIG_BPF_SYSCALL */
> > > };
> > >
> > > @@ -1954,6 +1969,8 @@ struct security_hook_heads {
> > > struct list_head bpf_map_free_security;
> > > struct list_head bpf_prog_alloc_security;
> > > struct list_head bpf_prog_free_security;
> > > + struct list_head bpf_map_file;
> > > + struct list_head bpf_prog_file;
> > > #endif /* CONFIG_BPF_SYSCALL */
> > > } __randomize_layout;
> > >
> > > diff --git a/include/linux/security.h b/include/linux/security.h
> > > index 18800b0911e5..57573b794e2d 100644
> > > --- a/include/linux/security.h
> > > +++ b/include/linux/security.h
> > > @@ -1740,6 +1740,8 @@ extern int security_bpf_map_alloc(struct
> > > bpf_map *map);
> > > extern void security_bpf_map_free(struct bpf_map *map);
> > > extern int security_bpf_prog_alloc(struct bpf_prog_aux *aux);
> > > extern void security_bpf_prog_free(struct bpf_prog_aux *aux);
> > > +extern void security_bpf_map_file(struct bpf_map *map, struct
> > > file
> > > *file);
> > > +extern void security_bpf_prog_file(struct bpf_prog_aux *aux,
> > > struct
> > > file *file);
> > > #else
> > > static inline int security_bpf(int cmd, union bpf_attr *attr,
> > > unsigned int size)
> > > @@ -1772,6 +1774,13 @@ static inline int
> > > security_bpf_prog_alloc(struct bpf_prog_aux *aux)
> > >
> > > static inline void security_bpf_prog_free(struct bpf_prog_aux
> > > *aux)
> > > { }
> > > +
> > > +static inline void security_bpf_map_file(struct bpf_map *map,
> > > struct
> > > file *file)
> > > +{ }
> > > +
> > > +static inline void security_bpf_prog_file(struct bpf_prog_aux
> > > *aux,
> > > + struct file *file)
> > > +{ }
> > > #endif /* CONFIG_SECURITY */
> > > #endif /* CONFIG_BPF_SYSCALL */
> > >
> > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > > index 1cf31ddd7616..b144181d3f3a 100644
> > > --- a/kernel/bpf/syscall.c
> > > +++ b/kernel/bpf/syscall.c
> > > @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file
> > > *filp,
> > > const char __user *buf,
> > > return -EINVAL;
> > > }
> > >
> > > -static const struct file_operations bpf_map_fops = {
> > > +const struct file_operations bpf_map_fops = {
> > > #ifdef CONFIG_PROC_FS
> > > .show_fdinfo = bpf_map_show_fdinfo,
> > > #endif
> > > @@ -964,7 +964,7 @@ static void bpf_prog_show_fdinfo(struct
> > > seq_file
> > > *m, struct file *filp)
> > > }
> > > #endif
> > >
> > > -static const struct file_operations bpf_prog_fops = {
> > > +const struct file_operations bpf_prog_fops = {
> > > #ifdef CONFIG_PROC_FS
> > > .show_fdinfo = bpf_prog_show_fdinfo,
> > > #endif
> > > diff --git a/security/security.c b/security/security.c
> > > index 1cd8526cb0b7..dacf649b8cfa 100644
> > > --- a/security/security.c
> > > +++ b/security/security.c
> > > @@ -1734,4 +1734,12 @@ void security_bpf_prog_free(struct
> > > bpf_prog_aux *aux)
> > > {
> > > call_void_hook(bpf_prog_free_security, aux);
> > > }
> > > +void security_bpf_map_file(struct bpf_map *map, struct file
> > > *file)
> > > +{
> > > + call_void_hook(bpf_map_file, map, file);
> > > +}
> > > +void security_bpf_prog_file(struct bpf_prog_aux *aux, struct
> > > file
> > > *file)
> > > +{
> > > + call_void_hook(bpf_prog_file, aux, file);
> > > +}
> > > #endif /* CONFIG_BPF_SYSCALL */
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 41aba4e3d57c..fea88655e0ee 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -1815,6 +1815,10 @@ static inline int file_path_has_perm(const
> > > struct cred *cred,
> > > return inode_has_perm(cred, file_inode(file), av, &ad);
> > > }
> > >
> > > +#ifdef CONFIG_BPF_SYSCALL
> > > +static int bpf_file_check(struct file *file, u32 sid);
> > > +#endif
> > > +
> > > /* Check whether a task can use an open file descriptor to
> > > access an inode in a given way. Check access to the
> > > descriptor itself, and then use dentry_has_perm to
> > > @@ -1845,6 +1849,12 @@ static int file_has_perm(const struct cred
> > > *cred,
> > > goto out;
> > > }
> > >
> > > +#ifdef CONFIG_BPF_SYSCALL
> > > + rc = bpf_file_check(file, cred_sid(cred));
> > > + if (rc)
> > > + goto out;
> > > +#endif
> > > +
> > > /* av is zero if only checking access to the descriptor. */
> > > rc = 0;
> > > if (av)
> > > @@ -2165,6 +2175,12 @@ static int
> > > selinux_binder_transfer_file(struct
> > > task_struct *from,
> > > return rc;
> > > }
> > >
> > > +#ifdef CONFIG_BPF_SYSCALL
> > > + rc = bpf_file_check(file, sid);
> > > + if (rc)
> > > + return rc;
> > > +#endif
> > > +
> > > if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> > > return 0;
> > >
> > > @@ -6288,6 +6304,33 @@ static u32 bpf_map_fmode_to_av(fmode_t
> > > fmode)
> > > return av;
> > > }
> > >
> > > +/* This function will check the file pass through unix socket or
> > > binder to see
> > > + * if it is a bpf related object. And apply correspinding checks
> > > on
> > > the bpf
> > > + * object based on the type. The bpf maps and programs, not like
> > > other files and
> > > + * socket, are using a shared anonymous inode inside the kernel
> > > as
> > > their inode.
> > > + * So checking that inode cannot identify if the process have
> > > privilege to
> > > + * access the bpf object and that's why we have to add this
> > > additional check in
> > > + * selinux_file_receive and selinux_binder_transfer_files.
> > > + */
> > > +static int bpf_file_check(struct file *file, u32 sid)
> > > +{
> > > + struct file_security_struct *fsec = file->f_security;
> > > + int ret;
> > > +
> > > + if (file->f_op == &bpf_map_fops) {
> > > + ret = avc_has_perm(sid, fsec->sid,
> > > SECCLASS_BPF_MAP,
> > > + bpf_map_fmode_to_av(file-
> > > > f_mode), NULL);
> > >
> > > + if (ret)
> > > + return ret;
> > > + } else if (file->f_op == &bpf_prog_fops) {
> > > + ret = avc_has_perm(sid, fsec->sid,
> > > SECCLASS_BPF_PROG,
> > > + BPF_PROG__USE, NULL);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
> > > {
> > > u32 sid = current_sid();
> > > @@ -6351,6 +6394,22 @@ static void selinux_bpf_prog_free(struct
> > > bpf_prog_aux *aux)
> > > aux->security = NULL;
> > > kfree(bpfsec);
> > > }
> > > +
> > > +static void selinux_bpf_map_file(struct bpf_map *map, struct
> > > file
> > > *file)
> > > +{
> > > + struct bpf_security_struct *bpfsec = map->security;
> > > + struct file_security_struct *fsec = file->f_security;
> > > +
> > > + fsec->sid = bpfsec->sid;
> > > +}
> > > +
> > > +static void selinux_bpf_prog_file(struct bpf_prog_aux *aux,
> > > struct
> > > file *file)
> > > +{
> > > + struct bpf_security_struct *bpfsec = aux->security;
> > > + struct file_security_struct *fsec = file->f_security;
> > > +
> > > + fsec->sid = bpfsec->sid;
> >
> > I could be wrong, but isn't it the case that fsec->sid already will
> > equal bpfsec->sid, because they are both created by the same thread
> > during the same system call, and they each inherit the SID of the
> > current task?
> >
>
> This is true when bpf object is created by the same process that
> obtains the fd. But there are other ways of getting a bpf object fd
> from the kernel such as bpf_obj_get and bpf_get_obj_fd_by_id. These
> action will ask the kernel to allocate a new file for the bpf object
> and the file sid would be the process ask for fd while the bpfsec-
> >sid
> is the sid when bpf object get created. These two could be different.
Oh, in that case you shouldn't change the fsec->sid; you'll need to use
the bpfsec->sid in your checks instead. But you can still do what I
described below.
> > What I expected you to do was to add and set a flags field in the
> > file_security_struct to indicate that this is a bpf map or prog,
> > and
> > then test for that in your bpf_file_check() function instead of
> > having
> > to export and test the fops structures.
> >
> >
> > > +}
> > > #endif
> > >
> > > static struct security_hook_list selinux_hooks[]
> > > __lsm_ro_after_init
> > > = {
> > > @@ -6581,6 +6640,8 @@ static struct security_hook_list
> > > selinux_hooks[] __lsm_ro_after_init = {
> > > LSM_HOOK_INIT(bpf_prog_alloc_security,
> > > selinux_bpf_prog_alloc),
> > > LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
> > > LSM_HOOK_INIT(bpf_prog_free_security,
> > > selinux_bpf_prog_free),
> > > + LSM_HOOK_INIT(bpf_map_file, selinux_bpf_map_file),
> > > + LSM_HOOK_INIT(bpf_prog_file, selinux_bpf_prog_file),
> > > #endif
> > > };
> > >
^ permalink raw reply
* Re: [PATCH][V2] ipv6: fix incorrect bitwise operator used on rt6i_flags
From: David Miller @ 2017-10-10 19:24 UTC (permalink / raw)
To: tracywwnj
Cc: colin.king, kuznet, yoshfuji, netdev, kernel-janitors,
linux-kernel
In-Reply-To: <CAC15z3jrkT6K0-FC9QLOmaZQCvEMoDZrABDJ_iGDecTYAksqAw@mail.gmail.com>
From: Wei Wang <tracywwnj@gmail.com>
Date: Tue, 10 Oct 2017 11:38:20 -0700
> On Tue, Oct 10, 2017 at 11:10 AM, Colin King <colin.king@canonical.com> wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The use of the | operator always leads to true which looks rather
>> suspect to me. Fix this by using & instead to just check the
>> RTF_CACHE entry bit.
>>
>> Detected by CoverityScan, CID#1457734, #1457747 ("Wrong operator used")
>>
>> Fixes: 35732d01fe31 ("ipv6: introduce a hash table to store dst cache")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>
> Acked-by: Wei Wang <weiwan@google.com>
Applied, thanks everyone.
^ permalink raw reply
* [PATCH 1/3] atm: idt77105: Drop needless setup_timer()
From: Kees Cook @ 2017-10-10 19:25 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Kees Cook, Chas Williams, linux-atm-general, netdev, linux-kernel
In-Reply-To: <1507663550-13343-1-git-send-email-keescook@chromium.org>
Calling setup_timer() is redundant when DEFINE_TIMER() has been used.
Cc: Chas Williams <3chas3@gmail.com>
Cc: linux-atm-general@lists.sourceforge.net
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
This is intended to go via the timer tree. Acks appreciated!
---
drivers/atm/idt77105.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/atm/idt77105.c b/drivers/atm/idt77105.c
index 57af9fd198e4..909744eb7bab 100644
--- a/drivers/atm/idt77105.c
+++ b/drivers/atm/idt77105.c
@@ -306,11 +306,9 @@ static int idt77105_start(struct atm_dev *dev)
if (start_timer) {
start_timer = 0;
- setup_timer(&stats_timer, idt77105_stats_timer_func, 0UL);
stats_timer.expires = jiffies+IDT77105_STATS_TIMER_PERIOD;
add_timer(&stats_timer);
- setup_timer(&restart_timer, idt77105_restart_timer_func, 0UL);
restart_timer.expires = jiffies+IDT77105_RESTART_TIMER_PERIOD;
add_timer(&restart_timer);
}
--
2.7.4
^ permalink raw reply related
* [PATCH] rtl8xxxu: mark expected switch fall-throughs
From: Gustavo A. R. Silva @ 2017-10-10 19:30 UTC (permalink / raw)
To: Jes Sorensen, Kalle Valo
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Gustavo A. R. Silva
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Cc: Jes Sorensen <Jes.Sorensen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Gustavo A. R. Silva <garsilva-L1vi/lXTdts+Va1GwOuvDg@public.gmane.org>
---
drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 7806a4d..e66be05 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -1153,6 +1153,7 @@ void rtl8xxxu_gen1_config_channel(struct ieee80211_hw *hw)
switch (hw->conf.chandef.width) {
case NL80211_CHAN_WIDTH_20_NOHT:
ht = false;
+ /* fall through */
case NL80211_CHAN_WIDTH_20:
opmode |= BW_OPMODE_20MHZ;
rtl8xxxu_write8(priv, REG_BW_OPMODE, opmode);
@@ -1280,6 +1281,7 @@ void rtl8xxxu_gen2_config_channel(struct ieee80211_hw *hw)
switch (hw->conf.chandef.width) {
case NL80211_CHAN_WIDTH_20_NOHT:
ht = false;
+ /* fall through */
case NL80211_CHAN_WIDTH_20:
rf_mode_bw |= WMAC_TRXPTCL_CTL_BW_20;
subchannel = 0;
@@ -1748,9 +1750,11 @@ static int rtl8xxxu_identify_chip(struct rtl8xxxu_priv *priv)
case 3:
priv->ep_tx_low_queue = 1;
priv->ep_tx_count++;
+ /* fall through */
case 2:
priv->ep_tx_normal_queue = 1;
priv->ep_tx_count++;
+ /* fall through */
case 1:
priv->ep_tx_high_queue = 1;
priv->ep_tx_count++;
@@ -5691,6 +5695,7 @@ static int rtl8xxxu_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
break;
case WLAN_CIPHER_SUITE_TKIP:
key->flags |= IEEE80211_KEY_FLAG_GENERATE_MMIC;
+ /* fall through */
default:
return -EOPNOTSUPP;
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next v2 0/7] bpf: get rid of global verifier state and reuse instruction printer
From: David Miller @ 2017-10-10 19:30 UTC (permalink / raw)
To: jakub.kicinski; +Cc: netdev, oss-drivers, alexei.starovoitov, daniel
In-Reply-To: <20171009173015.23520-1-jakub.kicinski@netronome.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Mon, 9 Oct 2017 10:30:08 -0700
> This set started off as simple extraction of eBPF verifier's instruction
> printer into a separate file but evolved into removal of global state.
> The purpose of moving instruction printing code is to be able to reuse it
> from the bpftool.
>
> As far as the global verifier lock goes, this set removes the global
> variables relating to the log buffer, makes the one-time init done
> by bpf_get_skb_set_tunnel_proto() not depend on any external locking,
> and performs verifier log writeback as data is produced removing the need
> for allocating a potentially large temporary buffer.
>
> The final step of actually removing the verifier lock is left to someone
> more competent and self-confident :)
>
> Note that struct bpf_verifier_env is just 40B under two pages now,
> we should probably switch to vzalloc() when it's expanded again...
>
> v2:
> - add a selftest;
> - use env buffer and flush on every print (Alexei);
> - handle kernel log allocation failures (Daniel);
> - put the env log members into a struct (Daniel).
Looks great, series applied, thanks Jakub.
^ permalink raw reply
* Re: [RFC net-next 1/4] net: ipv6: Make inet6addr_validator a blocking notifier
From: David Ahern @ 2017-10-10 19:32 UTC (permalink / raw)
To: netdev, idosch; +Cc: jiri, kjlx
In-Reply-To: <1507653665-20540-2-git-send-email-dsahern@gmail.com>
On 10/10/17 10:41 AM, David Ahern wrote:
> @@ -988,16 +987,23 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr,
> goto out2;
> }
>
> - i6vi.i6vi_addr = *addr;
> - i6vi.i6vi_dev = idev;
> - rcu_read_unlock_bh();
> + /* validator notifier needs to be blocking;
> + * do not call in softirq context
> + */
> + if (!in_softirq()) {
> + struct in6_validator_info i6vi = {
> + .i6vi_addr = *addr,
> + .i6vi_dev = idev,
> + };
>
> - err = inet6addr_validator_notifier_call_chain(NETDEV_UP, &i6vi);
> + rcu_read_unlock_bh();
> + err = inet6addr_validator_notifier_call_chain(NETDEV_UP, &i6vi);
> + rcu_read_lock_bh();
>
> - rcu_read_lock_bh();
> - err = notifier_to_errno(err);
> - if (err)
> - goto out2;
> + err = notifier_to_errno(err);
> + if (err)
> + goto out2;
> + }
>
> spin_lock(&addrconf_hash_lock);
>
The rcu_read_unlock_bh needs to be done before the in_softirq check.
With the change below I get the RIF overload with IPv6 addresses and I
verified the validator is skipped for RAs.
$ ip -batch vlan-ipv6-addr-batch
Error: spectrum: Exceeded number of supported router interfaces.
Command failed vlan-ipv6-addr-batch:683
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 0bad4a800f73..d9c5b29a3b8b 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -988,6 +988,8 @@ ipv6_add_addr(struct inet6_dev *idev, const struct
in6_addr *addr,
goto out2;
}
+ rcu_read_unlock_bh();
+
/* validator notifier needs to be blocking;
* do not call in softirq context
*/
@@ -998,15 +1000,14 @@ ipv6_add_addr(struct inet6_dev *idev, const
struct in6_addr *addr,
.extack = extack,
};
- rcu_read_unlock_bh();
err = inet6addr_validator_notifier_call_chain(NETDEV_UP,
&i6vi);
- rcu_read_lock_bh();
-
err = notifier_to_errno(err);
if (err)
- goto out2;
+ goto out1;
}
+ rcu_read_lock_bh();
+
spin_lock(&addrconf_hash_lock);
/* Ignore adding duplicate addresses on an interface */
@@ -1079,7 +1080,7 @@ ipv6_add_addr(struct inet6_dev *idev, const struct
in6_addr *addr,
write_unlock(&idev->lock);
out2:
rcu_read_unlock_bh();
-
+out1:
if (likely(err == 0))
inet6addr_notifier_call_chain(NETDEV_UP, ifa);
else {
^ permalink raw reply related
* Re: [PATCH v2] net/core: Fix BUG to BUG_ON conditionals.
From: David Miller @ 2017-10-10 19:32 UTC (permalink / raw)
To: devtimhansen
Cc: willemb, edumazet, soheil, pabeni, elena.reshetova, tom, Jason,
fw, netdev, linux-kernel, alexander.levin
In-Reply-To: <20171009153759.km3lqss7hb72wrze@debian>
From: Tim Hansen <devtimhansen@gmail.com>
Date: Mon, 9 Oct 2017 11:37:59 -0400
> Fix BUG() calls to use BUG_ON(conditional) macros.
>
> This was found using make coccicheck M=net/core on linux next
> tag next-2017092
>
> Signed-off-by: Tim Hansen <devtimhansen@gmail.com>
Althrough there were objections raised, none of them technically
stand up, and this does improve code generation for some
architectures, so I have applied this.
Thanks!
^ permalink raw reply
* Re: [PATCH] rtl8xxxu: mark expected switch fall-throughs
From: Jes Sorensen @ 2017-10-10 19:35 UTC (permalink / raw)
To: Gustavo A. R. Silva, Kalle Valo; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <20171010193027.GA23108@embeddedor.com>
On 10/10/2017 03:30 PM, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
While this isn't harmful, to me this looks like pointless patch churn
for zero gain and it's just ugly.
Jes
> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>
> Cc: Kalle Valo <kvalo@codeaurora.org>
> Cc: linux-wireless@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index 7806a4d..e66be05 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -1153,6 +1153,7 @@ void rtl8xxxu_gen1_config_channel(struct ieee80211_hw *hw)
> switch (hw->conf.chandef.width) {
> case NL80211_CHAN_WIDTH_20_NOHT:
> ht = false;
> + /* fall through */
> case NL80211_CHAN_WIDTH_20:
> opmode |= BW_OPMODE_20MHZ;
> rtl8xxxu_write8(priv, REG_BW_OPMODE, opmode);
> @@ -1280,6 +1281,7 @@ void rtl8xxxu_gen2_config_channel(struct ieee80211_hw *hw)
> switch (hw->conf.chandef.width) {
> case NL80211_CHAN_WIDTH_20_NOHT:
> ht = false;
> + /* fall through */
> case NL80211_CHAN_WIDTH_20:
> rf_mode_bw |= WMAC_TRXPTCL_CTL_BW_20;
> subchannel = 0;
> @@ -1748,9 +1750,11 @@ static int rtl8xxxu_identify_chip(struct rtl8xxxu_priv *priv)
> case 3:
> priv->ep_tx_low_queue = 1;
> priv->ep_tx_count++;
> + /* fall through */
> case 2:
> priv->ep_tx_normal_queue = 1;
> priv->ep_tx_count++;
> + /* fall through */
> case 1:
> priv->ep_tx_high_queue = 1;
> priv->ep_tx_count++;
> @@ -5691,6 +5695,7 @@ static int rtl8xxxu_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
> break;
> case WLAN_CIPHER_SUITE_TKIP:
> key->flags |= IEEE80211_KEY_FLAG_GENERATE_MMIC;
> + /* fall through */
> default:
> return -EOPNOTSUPP;
> }
>
^ 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