* Re: [PATCH][next] sctp: make array sctp_sched_ops static
From: Joe Perches @ 2017-10-11 10:44 UTC (permalink / raw)
To: Colin King, Vlad Yasevich, Neil Horman, David S . Miller,
linux-sctp, netdev
Cc: kernel-janitors, linux-kernel
In-Reply-To: <20171011101757.18825-1-colin.king@canonical.com>
On Wed, 2017-10-11 at 11:17 +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The array sctp_sched_ops is local to the source and
> does not need to be in global scope, so make it static.
>
> Cleans up sparse warning:
> symbol 'sctp_sched_ops' was not declared. Should it be static?
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> net/sctp/stream_sched.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sctp/stream_sched.c b/net/sctp/stream_sched.c
> index 03513a9fa110..0b83ec51e43b 100644
> --- a/net/sctp/stream_sched.c
> +++ b/net/sctp/stream_sched.c
> @@ -124,7 +124,7 @@ static struct sctp_sched_ops sctp_sched_fcfs = {
> extern struct sctp_sched_ops sctp_sched_prio;
> extern struct sctp_sched_ops sctp_sched_rr;
>
> -struct sctp_sched_ops *sctp_sched_ops[] = {
> +static struct sctp_sched_ops *sctp_sched_ops[] = {
> &sctp_sched_fcfs,
> &sctp_sched_prio,
> &sctp_sched_rr,
Perhaps these should also be const to move more data to text
---
include/net/sctp/stream_sched.h | 3 ++-
include/net/sctp/structs.h | 2 +-
net/sctp/stream.c | 6 +++---
net/sctp/stream_sched.c | 17 +++++++++--------
4 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/include/net/sctp/stream_sched.h b/include/net/sctp/stream_sched.h
index c676550a4c7d..431235c7587a 100644
--- a/include/net/sctp/stream_sched.h
+++ b/include/net/sctp/stream_sched.h
@@ -67,6 +67,7 @@ void sctp_sched_dequeue_done(struct sctp_outq *q, struct sctp_chunk *ch);
void sctp_sched_dequeue_common(struct sctp_outq *q, struct sctp_chunk *ch);
int sctp_sched_init_sid(struct sctp_stream *stream, __u16 sid, gfp_t gfp);
-struct sctp_sched_ops *sctp_sched_ops_from_stream(struct sctp_stream *stream);
+const struct sctp_sched_ops *
+sctp_sched_ops_from_stream(struct sctp_stream *stream);
#endif /* __sctp_stream_sched_h__ */
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 6168e3449131..032ec5618e8a 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1028,7 +1028,7 @@ struct sctp_outq {
struct list_head out_chunk_list;
/* Stream scheduler being used */
- struct sctp_sched_ops *sched;
+ const struct sctp_sched_ops *sched;
unsigned int out_qlen; /* Total length of queued data chunks. */
diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index 5ea33a2c453b..62678c2229e4 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -140,7 +140,7 @@ static int sctp_stream_alloc_in(struct sctp_stream *stream, __u16 incnt,
int sctp_stream_init(struct sctp_stream *stream, __u16 outcnt, __u16 incnt,
gfp_t gfp)
{
- struct sctp_sched_ops *sched = sctp_sched_ops_from_stream(stream);
+ const struct sctp_sched_ops *sched = sctp_sched_ops_from_stream(stream);
int i, ret = 0;
gfp |= __GFP_NOWARN;
@@ -201,7 +201,7 @@ int sctp_stream_init_ext(struct sctp_stream *stream, __u16 sid)
void sctp_stream_free(struct sctp_stream *stream)
{
- struct sctp_sched_ops *sched = sctp_sched_ops_from_stream(stream);
+ const struct sctp_sched_ops *sched = sctp_sched_ops_from_stream(stream);
int i;
sched->free(stream);
@@ -224,7 +224,7 @@ void sctp_stream_clear(struct sctp_stream *stream)
void sctp_stream_update(struct sctp_stream *stream, struct sctp_stream *new)
{
- struct sctp_sched_ops *sched = sctp_sched_ops_from_stream(stream);
+ const struct sctp_sched_ops *sched = sctp_sched_ops_from_stream(stream);
sched->unsched_all(stream);
sctp_stream_outq_migrate(stream, new, new->outcnt);
diff --git a/net/sctp/stream_sched.c b/net/sctp/stream_sched.c
index 03513a9fa110..642542be57ec 100644
--- a/net/sctp/stream_sched.c
+++ b/net/sctp/stream_sched.c
@@ -106,7 +106,7 @@ static void sctp_sched_fcfs_unsched_all(struct sctp_stream *stream)
{
}
-static struct sctp_sched_ops sctp_sched_fcfs = {
+static const struct sctp_sched_ops sctp_sched_fcfs = {
.set = sctp_sched_fcfs_set,
.get = sctp_sched_fcfs_get,
.init = sctp_sched_fcfs_init,
@@ -121,10 +121,10 @@ static struct sctp_sched_ops sctp_sched_fcfs = {
/* API to other parts of the stack */
-extern struct sctp_sched_ops sctp_sched_prio;
-extern struct sctp_sched_ops sctp_sched_rr;
+extern const struct sctp_sched_ops sctp_sched_prio;
+extern const struct sctp_sched_ops sctp_sched_rr;
-struct sctp_sched_ops *sctp_sched_ops[] = {
+static const struct sctp_sched_ops *sctp_sched_ops[] = {
&sctp_sched_fcfs,
&sctp_sched_prio,
&sctp_sched_rr,
@@ -133,8 +133,8 @@ struct sctp_sched_ops *sctp_sched_ops[] = {
int sctp_sched_set_sched(struct sctp_association *asoc,
enum sctp_sched_type sched)
{
- struct sctp_sched_ops *n = sctp_sched_ops[sched];
- struct sctp_sched_ops *old = asoc->outqueue.sched;
+ const struct sctp_sched_ops *n = sctp_sched_ops[sched];
+ const struct sctp_sched_ops *old = asoc->outqueue.sched;
struct sctp_datamsg *msg = NULL;
struct sctp_chunk *ch;
int i, ret = 0;
@@ -259,13 +259,14 @@ void sctp_sched_dequeue_common(struct sctp_outq *q, struct sctp_chunk *ch)
int sctp_sched_init_sid(struct sctp_stream *stream, __u16 sid, gfp_t gfp)
{
- struct sctp_sched_ops *sched = sctp_sched_ops_from_stream(stream);
+ const struct sctp_sched_ops *sched = sctp_sched_ops_from_stream(stream);
INIT_LIST_HEAD(&stream->out[sid].ext->outq);
return sched->init_sid(stream, sid, gfp);
}
-struct sctp_sched_ops *sctp_sched_ops_from_stream(struct sctp_stream *stream)
+const struct sctp_sched_ops *
+sctp_sched_ops_from_stream(struct sctp_stream *stream)
{
struct sctp_association *asoc;
^ permalink raw reply related
* Re: [net-next 2/3] ip_gre: fix erspan tunnel mtu calculation
From: Xin Long @ 2017-10-11 10:46 UTC (permalink / raw)
To: William Tu; +Cc: davem, Linux Kernel Network Developers
In-Reply-To: <CALDO+SZhdSXQB-n_o006_fOGnmz1j99uJBAOVixD0YCFgKnVFw@mail.gmail.com>
On Tue, Oct 10, 2017 at 8:59 PM, William Tu <u9012063@gmail.com> wrote:
>>> @@ -1242,14 +1241,14 @@ static int erspan_tunnel_init(struct net_device *dev)
>>> struct ip_tunnel *tunnel = netdev_priv(dev);
>>> int t_hlen;
>>>
>>> - tunnel->tun_hlen = 8;
>>> + tunnel->tun_hlen = ERSPAN_GREHDR_LEN;
>>> tunnel->parms.iph.protocol = IPPROTO_GRE;
>>> tunnel->hlen = tunnel->tun_hlen + tunnel->encap_hlen +
>>> sizeof(struct erspanhdr);
>>> t_hlen = tunnel->hlen + sizeof(struct iphdr);
>>>
>>> - dev->needed_headroom = LL_MAX_HEADER + t_hlen + 4;
>>> - dev->mtu = ETH_DATA_LEN - t_hlen - 4;
>>> + dev->needed_headroom = LL_MAX_HEADER + t_hlen;
>>> + dev->mtu = ETH_DATA_LEN - t_hlen;
>> 1. I guess '+4-4' stuff was copied from __gre_tunnel_init(), I'm thinking
>> it may be there for some reason.
>>
> I traced back to
> 4565e9919cda ("gre: Setup and TX path for gre/UDP foo-over-udp encapsulation")
> and I think '+4-4' is there for GRE base header length.
>
> Since now we do
> dev->mtu = ETH_DATA_LEN - t_hlen;
> and t_hlen already counts the the gre base header + optional header
> len, I think it's not needed.
okay. thanks.
>
>> 2. 'dev->needed_headroom =' and 'dev->mtu =' are really needed ?
>> As I've seen both will be updated in .newlink:
>> ipgre_newlink() -> ip_tunnel_newlink() -> ip_tunnel_bind_dev()
>>
> right, I also find both values gets overwritten by
> ip_tunnel_bind_dev() using my test cases. Maybe we can remove them?
It's there just in case that there is no lower dev found, but
ip_tunnel_newlink/create always updates dev->mtu even if
there is no lower dev found.
let's leave as it is for now, ipgre may just not be sure ip_tunnel_xxx would
do it when no lower dev found.
^ permalink raw reply
* [PATCH][bpf-next] bpf: remove redundant variable old_flags
From: Colin King @ 2017-10-11 10:56 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, netdev; +Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Variable old_flags is being assigned but is never read; it is redundant
and can be removed.
Cleans up clang warning: Value stored to 'old_flags' is never read
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
kernel/bpf/cgroup.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index e88abc0865d5..3db5a17fcfe8 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -192,7 +192,6 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
struct cgroup_subsys_state *css;
struct bpf_prog_list *pl;
bool pl_was_allocated;
- u32 old_flags;
int err;
if ((flags & BPF_F_ALLOW_OVERRIDE) && (flags & BPF_F_ALLOW_MULTI))
@@ -239,7 +238,6 @@ int __cgroup_bpf_attach(struct cgroup *cgrp, struct bpf_prog *prog,
pl->prog = prog;
}
- old_flags = cgrp->bpf.flags[type];
cgrp->bpf.flags[type] = flags;
/* allocate and recompute effective prog arrays */
--
2.14.1
^ permalink raw reply related
* Re: [PATCHv4 iproute2 2/2] lib/libnetlink: update rtnl_talk to support malloc buff at run time
From: Phil Sutter @ 2017-10-11 11:10 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Michal Kubecek, Hangbin Liu, netdev, Hangbin Liu
In-Reply-To: <20171010094743.6ae2baa8@shemminger-XPS-13-9360>
On Tue, Oct 10, 2017 at 09:47:43AM -0700, Stephen Hemminger wrote:
> On Tue, 10 Oct 2017 08:41:17 +0200
> Michal Kubecek <mkubecek@suse.cz> wrote:
>
> > On Mon, Oct 09, 2017 at 10:25:25PM +0200, Phil Sutter wrote:
> > > Hi Stephen,
> > >
> > > On Mon, Oct 02, 2017 at 10:37:08AM -0700, Stephen Hemminger wrote:
> > > > On Thu, 28 Sep 2017 21:33:46 +0800
> > > > Hangbin Liu <haliu@redhat.com> wrote:
> > > >
> > > > > From: Hangbin Liu <liuhangbin@gmail.com>
> > > > >
> > > > > This is an update for 460c03f3f3cc ("iplink: double the buffer size also in
> > > > > iplink_get()"). After update, we will not need to double the buffer size
> > > > > every time when VFs number increased.
> > > > >
> > > > > With call like rtnl_talk(&rth, &req.n, NULL, 0), we can simply remove the
> > > > > length parameter.
> > > > >
> > > > > With call like rtnl_talk(&rth, nlh, nlh, sizeof(req), I add a new variable
> > > > > answer to avoid overwrite data in nlh, because it may has more info after
> > > > > nlh. also this will avoid nlh buffer not enough issue.
> > > > >
> > > > > We need to free answer after using.
> > > > >
> > > > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > > > ---
> > > >
> > > > Most of the uses of rtnl_talk() don't need to this peek and dynamic sizing.
> > > > Can only those places that need that be targeted?
> > >
> > > We could probably do that, by having a buffer on stack in __rtnl_talk()
> > > which will be used instead of the allocated one if 'answer' is NULL. Or
> > > maybe even introduce a dedicated API call for the dynamically allocated
> > > receive buffer. But I really doubt that's feasible: AFAICT, that stack
> > > buffer still needs to be reasonably sized since the reply might be
> > > larger than the request (reusing the request buffer would be the most
> > > simple way to tackle this), also there is support for extack which may
> > > bloat the response to arbitrary size. Hangbin has shown in his benchmark
> > > that the overhead of the second syscall is negligible, so why care about
> > > that and increase code complexity even further?
> > >
> > > Not saying it's not possible, but I just doubt it's worth the effort.
> >
> > Agreed. Current code is based on the assumption that we can estimate the
> > maximum reply length in advance and the reason for this series is that
> > this assumption turned out to be wrong. I'm afraid that if we replace
> > it by an assumption that we can estimate the maximum reply length for
> > most requests with only few exceptions, it's only matter of time for us
> > to be proven wrong again.
> >
> > Michal Kubecek
> >
>
> For query responses, yes the response may be large. But for the common cases of
> add address or add route, the response should just be ack or error.
And with extack, error is comprised of the original request plus an
arbitrarily sized error message, so we can't just reuse the request
buffer and are back to "guessing" the right length again.
To get an idea of what we're talking about, I wrote a simple benchmark
which adds 256 * 254 (= 65024) addresses to an interface, then removes
them again one by one and measured the time that takes for binaries with
and without Hangbin's patches:
OP Vanilla Hangbin Delta
--------------------------------------------------------
add real 2m16.244s real 2m27.964s +11.72s (108.6%)
user 0m15.241s user 0m17.295s +2.054s (113.5%)
sys 1m40.229s sys 1m48.239s +8.01s (108.0%)
remove real 1m44.950s real 1m47.044s +2.094s (102.0%)
user 0m13.899s user 0m14.723s +0.824s (105.9%)
sys 1m30.798s sys 1m31.938s +1.140s (101.3%)
So the overhead of the second syscall and dynamic memory allocation is
less than 10% overall. Given the short time a single call to 'ip'
typically takes, I don't think the difference is noticeable even in
highly performance critical applications.
Cheers, Phil
^ permalink raw reply
* Re: [PATCH][next] sctp: make array sctp_sched_ops static
From: Neil Horman @ 2017-10-11 11:39 UTC (permalink / raw)
To: Colin King
Cc: Vlad Yasevich, David S . Miller, linux-sctp, netdev,
kernel-janitors, linux-kernel
In-Reply-To: <20171011101757.18825-1-colin.king@canonical.com>
On Wed, Oct 11, 2017 at 11:17:57AM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The array sctp_sched_ops is local to the source and
> does not need to be in global scope, so make it static.
>
> Cleans up sparse warning:
> symbol 'sctp_sched_ops' was not declared. Should it be static?
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> net/sctp/stream_sched.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sctp/stream_sched.c b/net/sctp/stream_sched.c
> index 03513a9fa110..0b83ec51e43b 100644
> --- a/net/sctp/stream_sched.c
> +++ b/net/sctp/stream_sched.c
> @@ -124,7 +124,7 @@ static struct sctp_sched_ops sctp_sched_fcfs = {
> extern struct sctp_sched_ops sctp_sched_prio;
> extern struct sctp_sched_ops sctp_sched_rr;
>
> -struct sctp_sched_ops *sctp_sched_ops[] = {
> +static struct sctp_sched_ops *sctp_sched_ops[] = {
> &sctp_sched_fcfs,
> &sctp_sched_prio,
> &sctp_sched_rr,
> --
> 2.14.1
>
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* [PATCH v2 net 0/2] net/smc: ib_query_gid() patches
From: Ursula Braun @ 2017-10-11 11:47 UTC (permalink / raw)
To: davem-fT/PcQaiUtIeIZ0/mPfg9Q
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
linux-s390-u79uwXL29TY76Z2rM5mHXA,
jwi-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
schwidefsky-tA70FqPdS9bQT0dZR+AlfA,
heiko.carstens-tA70FqPdS9bQT0dZR+AlfA,
raspl-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
ubraun-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
parav-VPRAkNaXOzVWk0Htik3J/w
Dave,
triggered by Parav Pandit here are 2 cleanup patches for usage of
ib_query_gid() in the smc-code.
Thanks, Ursula
v2 changes advised by Parav Pandit:
extra check is_vlan_dev() in patch 2/2
"RoCE" spelling
added "Reported-by"
added "Reviewed-by"
added "Fixes"
Ursula Braun (2):
net/smc: replace function pointer get_netdev()
net/smc: dev_put for netdev after usage of ib_query_gid()
net/smc/smc_core.c | 12 ++++++++----
net/smc/smc_ib.c | 26 +++++++++-----------------
2 files changed, 17 insertions(+), 21 deletions(-)
--
2.13.5
--
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
* [PATCH v2 net 1/2] net/smc: replace function pointer get_netdev()
From: Ursula Braun @ 2017-10-11 11:47 UTC (permalink / raw)
To: davem
Cc: netdev, linux-rdma, linux-s390, jwi, schwidefsky, heiko.carstens,
raspl, ubraun, parav
In-Reply-To: <20171011114723.30733-1-ubraun@linux.vnet.ibm.com>
SMC should not open code the function pointer get_netdev of the
IB device. Replacing ib_query_gid(..., NULL) with
ib_query_gid(..., gid_attr) allows access to the netdev.
Signed-off-by: Ursula Braun <ubraun@linux.vnet.ibm.com>
Suggested-by: Parav Pandit <parav@mellanox.com>
Reviewed-by: Parav Pandit <parav@mellanox.com>
---
net/smc/smc_ib.c | 26 +++++++++-----------------
1 file changed, 9 insertions(+), 17 deletions(-)
diff --git a/net/smc/smc_ib.c b/net/smc/smc_ib.c
index 0b5852299158..468e1d725d97 100644
--- a/net/smc/smc_ib.c
+++ b/net/smc/smc_ib.c
@@ -369,26 +369,17 @@ void smc_ib_buf_unmap_sg(struct smc_ib_device *smcibdev,
static int smc_ib_fill_gid_and_mac(struct smc_ib_device *smcibdev, u8 ibport)
{
- struct net_device *ndev;
+ struct ib_gid_attr gattr;
int rc;
rc = ib_query_gid(smcibdev->ibdev, ibport, 0,
- &smcibdev->gid[ibport - 1], NULL);
- /* the SMC protocol requires specification of the roce MAC address;
- * if net_device cannot be determined, it can be derived from gid 0
- */
- ndev = smcibdev->ibdev->get_netdev(smcibdev->ibdev, ibport);
- if (ndev) {
- memcpy(&smcibdev->mac, ndev->dev_addr, ETH_ALEN);
- dev_put(ndev);
- } else if (!rc) {
- memcpy(&smcibdev->mac[ibport - 1][0],
- &smcibdev->gid[ibport - 1].raw[8], 3);
- memcpy(&smcibdev->mac[ibport - 1][3],
- &smcibdev->gid[ibport - 1].raw[13], 3);
- smcibdev->mac[ibport - 1][0] &= ~0x02;
- }
- return rc;
+ &smcibdev->gid[ibport - 1], &gattr);
+ if (rc || !gattr.ndev)
+ return -ENODEV;
+
+ memcpy(smcibdev->mac[ibport - 1], gattr.ndev->dev_addr, ETH_ALEN);
+ dev_put(gattr.ndev);
+ return 0;
}
/* Create an identifier unique for this instance of SMC-R.
@@ -419,6 +410,7 @@ int smc_ib_remember_port_attr(struct smc_ib_device *smcibdev, u8 ibport)
&smcibdev->pattr[ibport - 1]);
if (rc)
goto out;
+ /* the SMC protocol requires specification of the RoCE MAC address */
rc = smc_ib_fill_gid_and_mac(smcibdev, ibport);
if (rc)
goto out;
--
2.13.5
^ permalink raw reply related
* [PATCH v2 net 2/2] net/smc: dev_put for netdev after usage of ib_query_gid()
From: Ursula Braun @ 2017-10-11 11:47 UTC (permalink / raw)
To: davem
Cc: netdev, linux-rdma, linux-s390, jwi, schwidefsky, heiko.carstens,
raspl, ubraun, parav
In-Reply-To: <20171011114723.30733-1-ubraun@linux.vnet.ibm.com>
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@linux.vnet.ibm.com>
Reported-by: Parav Pandit <parav@mellanox.com>
Reviewed-by: Parav Pandit <parav@mellanox.com>
Fixes: 0cfdd8f92cac ("smc: connection and link group creation")
---
net/smc/smc_core.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 20b66e79c5d6..5f6a20084157 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -380,10 +380,14 @@ static int smc_link_determine_gid(struct smc_link_group *lgr)
if (ib_query_gid(lnk->smcibdev->ibdev, lnk->ibport, i, &gid,
&gattr))
continue;
- if (gattr.ndev &&
- (vlan_dev_vlan_id(gattr.ndev) == lgr->vlan_id)) {
- lnk->gid = gid;
- return 0;
+ if (gattr.ndev) {
+ if (is_vlan_dev(gattr.ndev) &&
+ vlan_dev_vlan_id(gattr.ndev) == lgr->vlan_id) {
+ lnk->gid = gid;
+ dev_put(gattr.ndev);
+ return 0;
+ }
+ dev_put(gattr.ndev);
}
}
return -ENODEV;
--
2.13.5
^ permalink raw reply related
* Re: ipsec: Fix dst leak in xfrm_bundle_create().
From: Steffen Klassert @ 2017-10-11 11:54 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20171010.205938.2305003344238226004.davem@davemloft.net>
On Tue, Oct 10, 2017 at 08:59:38PM -0700, David Miller wrote:
>
> If we cannot find a suitable inner_mode value, we will leak
> the currently allocated 'xdst'.
>
> The fix is to make sure it is linked into the chain before
> erroring out.
>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
>
> Steffen, I found this via visual inspection. Please double check my
> work before applying this :-)
Looks good. Patch applied, thanks David!
^ permalink raw reply
* Re: [PATCH net-next 1/2] net sched act_vlan: Change stats update to use per-core stats
From: Jamal Hadi Salim @ 2017-10-11 12:27 UTC (permalink / raw)
To: Manish Kurup, xiyou.wangcong, jiri, davem, netdev
Cc: aring, mrv, manish.kurup
In-Reply-To: <1507689147-22941-1-git-send-email-manish.kurup@verizon.com>
minus lk
On 17-10-10 10:32 PM, Manish Kurup wrote:
> The VLAN action maintains one set of stats across all cores, and uses a
> spinlock to synchronize updates to it from the same. Changed this to use a
> per-CPU stats context instead.
> This change will result in better performance.
>
> Signed-off-by: Manish Kurup <manish.kurup@verizon.com>
> ---
> net/sched/act_vlan.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
> index 16eb067..14c262c 100644
> --- a/net/sched/act_vlan.c
> +++ b/net/sched/act_vlan.c
> @@ -30,9 +30,10 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
> int err;
> u16 tci;
>
> - spin_lock(&v->tcf_lock);
> tcf_lastuse_update(&v->tcf_tm);
> - bstats_update(&v->tcf_bstats, skb);
> + bstats_cpu_update(this_cpu_ptr(v->common.cpu_bstats), skb);
> +
> + spin_lock(&v->tcf_lock);
> action = v->tcf_action;
>
> /* Ensure 'data' points at mac_header prior calling vlan manipulating
> @@ -85,7 +86,8 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
>
> drop:
> action = TC_ACT_SHOT;
> - v->tcf_qstats.drops++;
> + qstats_drop_inc(this_cpu_ptr(v->common.cpu_qstats));
> +
> unlock:
> if (skb_at_tc_ingress(skb))
> skb_pull_rcsum(skb, skb->mac_len);
> @@ -172,7 +174,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
>
> if (!exists) {
> ret = tcf_idr_create(tn, parm->index, est, a,
> - &act_vlan_ops, bind, false);
> + &act_vlan_ops, bind, true);
>
Indentation mismatch here?
Otherwise looks good to me.
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
cheers,
jamal
^ permalink raw reply
* Re: [PATCH net-next 2/2] net sched act_vlan: VLAN action rewrite to use RCU lock/unlock and update
From: Jamal Hadi Salim @ 2017-10-11 12:28 UTC (permalink / raw)
To: Manish Kurup, xiyou.wangcong, jiri, davem, netdev, linux-kernel
Cc: aring, mrv, manish.kurup
In-Reply-To: <1507689219-22993-1-git-send-email-manish.kurup@verizon.com>
On 17-10-10 10:33 PM, Manish Kurup wrote:
> Using a spinlock in the VLAN action causes performance issues when the VLAN
> action is used on multiple cores. Rewrote the VLAN action to use RCU read
> locking for reads and updates instead.
>
> Signed-off-by: Manish Kurup <manish.kurup@verizon.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
cheers,
jamal
^ permalink raw reply
* Re: [PATCH net v2] net: enable interface alias removal via rtnl
From: Nicolas Dichtel @ 2017-10-11 12:29 UTC (permalink / raw)
To: David Ahern, davem; +Cc: netdev, oliver, Stephen Hemminger
In-Reply-To: <c81359e7-feb8-475e-69d7-3eacc3406caf@gmail.com>
Le 10/10/2017 à 16:50, David Ahern a écrit :
> On 10/10/17 6:41 AM, Nicolas Dichtel wrote:
>> IFLA_IFALIAS is defined as NLA_STRING. It means that the minimal length of
>> the attribute is 1 ("\0"). However, to remove an alias, the attribute
>> length must be 0 (see dev_set_alias()).
>>
>> Let's define the type to NLA_BINARY, so that the alias can be removed.
>
> not to be pedantic, but we need to be clear that the type is changed
> only for policy validation.
With the comment in the code, it is clear, isn't it?
Regards,
Nicolas
^ permalink raw reply
* pull-request: mac80211-next 2017-10-11
From: Johannes Berg @ 2017-10-11 12:36 UTC (permalink / raw)
To: David Miller
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA
Hi Dave,
Here's a -next pull request. The only bigger thing here is the
addition of the regulatory database as firmware, which will
allow us to - over time - get rid of CRDA, as well as having
the option of adding more fields to the database where needed,
this would've been extremely complex with CRDA because it had
not been built with extensibility in mind.
Please pull and let me know if there's any problem.
Thanks,
johannes
The following changes since commit cc71b7b071192ac1c288e272fdc3f3877eb96663:
net/ipv6: remove unused err variable on icmpv6_push_pending_frames (2017-10-05 21:56:26 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git tags/mac80211-next-for-davem-2017-10-11
for you to fetch changes up to 90a53e4432b12288316efaa5f308adafb8d304b0:
cfg80211: implement regdb signature checking (2017-10-11 14:24:24 +0200)
----------------------------------------------------------------
Work continues in various areas:
* port authorized event for 4-way-HS offload (Avi)
* enable MFP optional for such devices (Emmanuel)
* Kees's timer setup patch for mac80211 mesh
(the part that isn't trivially scripted)
* improve VLAN vs. TXQ handling (myself)
* load regulatory database as firmware file (myself)
* with various other small improvements and cleanups
I merged net-next once in the meantime to allow Kees's
timer setup patch to go in.
----------------------------------------------------------------
Avraham Stern (2):
ieee80211: Add WFA TPC report element OUI type
cfg80211/nl80211: add a port authorized event
Emmanuel Grumbach (1):
nl80211: add an option to allow MFP without requiring it
Gregory Greenman (1):
mac80211: recalculate some sta parameters after insertion
Ilan peer (1):
mac80211: Simplify locking in ieee80211_sta_tear_down_BA_sessions()
Johannes Berg (12):
mac80211: avoid allocating TXQs that won't be used
mac80211: simplify and clarify IE splitting
mac80211: use offsetofend()
cfg80211: remove unused function ieee80211_data_from_8023()
Merge remote-tracking branch 'net-next/master' into mac80211-next
MAINTAINERS: update Johannes Berg's entries
fq: support filtering a given tin
mac80211: only remove AP VLAN frames from TXQ
cfg80211: support loading regulatory database as firmware file
cfg80211: support reloading regulatory database
cfg80211: reg: remove support for built-in regdb
cfg80211: implement regdb signature checking
Kees Cook (1):
net/mac80211/mesh_plink: Convert timers to use timer_setup()
Liad Kaufman (1):
mac80211: extend ieee80211_ie_split to support EXTENSION
Lubomir Rintel (1):
mac80211_hwsim: use dyndbg for debug messages
Luca Coelho (1):
mac80211: add documentation to ieee80211_rx_ba_offl()
Manikanta Pubbisetty (1):
mac80211: fix bandwidth computation for TDLS peers
Richard Schütz (1):
wireless: set correct mandatory rate flags
Roee Zamir (2):
nl80211: add OCE scan and capability flags
mac80211: oce: enable receiving of bcast probe resp
Stanislaw Gruszka (1):
mac80211: fix STA_SLOW_THRESHOLD htmldocs failure
Tova Mussai (1):
nl80211: return error for invalid center_freq in 40 MHz
Xiang Gao (1):
mac80211: aead api to reduce redundancy
Documentation/driver-api/80211/cfg80211.rst | 3 -
Documentation/networking/regulatory.txt | 30 +-
MAINTAINERS | 13 +-
drivers/net/wireless/mac80211_hwsim.c | 192 ++++++------
include/linux/ieee80211.h | 1 +
include/net/cfg80211.h | 40 +--
include/net/fq.h | 7 +
include/net/fq_impl.h | 72 ++++-
include/net/mac80211.h | 8 +-
include/uapi/linux/nl80211.h | 82 +++--
net/mac80211/Makefile | 3 +-
net/mac80211/{aes_ccm.c => aead_api.c} | 40 +--
net/mac80211/aead_api.h | 27 ++
net/mac80211/aes_ccm.h | 42 ++-
net/mac80211/aes_gcm.c | 109 -------
net/mac80211/aes_gcm.h | 38 ++-
net/mac80211/agg-rx.c | 4 +-
net/mac80211/ht.c | 12 +-
net/mac80211/ieee80211_i.h | 2 +
net/mac80211/iface.c | 29 +-
net/mac80211/mesh.c | 3 +-
net/mac80211/mesh.h | 1 +
net/mac80211/mesh_hwmp.c | 8 +-
net/mac80211/mesh_plink.c | 13 +-
net/mac80211/mlme.c | 19 +-
net/mac80211/scan.c | 37 ++-
net/mac80211/sta_info.c | 61 ++--
net/mac80211/sta_info.h | 4 +-
net/mac80211/tx.c | 34 +++
net/mac80211/util.c | 25 +-
net/mac80211/vht.c | 10 +
net/mac80211/wpa.c | 4 +-
net/wireless/.gitignore | 3 +-
net/wireless/Kconfig | 58 ++--
net/wireless/Makefile | 24 +-
net/wireless/certs/sforshee.x509 | Bin 0 -> 680 bytes
net/wireless/core.c | 2 +-
net/wireless/core.h | 5 +
net/wireless/db.txt | 17 --
net/wireless/genregdb.awk | 158 ----------
net/wireless/nl80211.c | 199 ++++++++----
net/wireless/nl80211.h | 2 +
net/wireless/reg.c | 452 +++++++++++++++++++++++++---
net/wireless/reg.h | 14 +
net/wireless/regdb.h | 23 --
net/wireless/sme.c | 45 ++-
net/wireless/util.c | 202 ++++---------
47 files changed, 1278 insertions(+), 899 deletions(-)
rename net/mac80211/{aes_ccm.c => aead_api.c} (67%)
create mode 100644 net/mac80211/aead_api.h
delete mode 100644 net/mac80211/aes_gcm.c
create mode 100644 net/wireless/certs/sforshee.x509
delete mode 100644 net/wireless/db.txt
delete mode 100644 net/wireless/genregdb.awk
delete mode 100644 net/wireless/regdb.h
^ permalink raw reply
* Re: [jkirsher/next-queue PATCH v4 0/6] tc-flower based cloud filters in i40e
From: Jamal Hadi Salim @ 2017-10-11 12:42 UTC (permalink / raw)
To: Amritha Nambiar, intel-wired-lan, jeffrey.t.kirsher
Cc: alexander.h.duyck, jiri, netdev, alexander.duyck, xiyou.wangcong
In-Reply-To: <150768099999.5320.1633617713417675266.stgit@anamdev.jf.intel.com>
On 17-10-10 08:24 PM, Amritha Nambiar wrote:
> This patch series enables configuring cloud filters in i40e
> using the tc-flower classifier. The classification function
> of the filter is to match a packet to a class. cls_flower is
> extended to offload classid to hardware. The offloaded classid
> is used direct matched packets to a traffic class on the device.
> The approach here is similar to the tc 'prio' qdisc which uses
> the classid for band selection. The ingress qdisc is called ffff:0,
> so traffic classes are ffff:1 to ffff:8 (i40e has max of 8 TCs).
> TC0 is minor number 1, TC1 is minor number 2 etc.
>
> The cloud filters are added for a VSI and are cleaned up when
> the VSI is deleted. The filters that match on L4 ports needs
> enhanced admin queue functions with big buffer support for
> extended fields in cloud filter commands.
>
> Example:
> # tc qdisc add dev eth0 ingress
> # ethtool -K eth0 hw-tc-offload on
>
> Match Dst IPv4,Dst Port and route to TC1:
> # tc filter add dev eth0 protocol ip parent ffff: prio 1 flower\
> dst_ip 192.168.1.1/32 ip_proto udp dst_port 22\
> skip_sw classid ffff:2
>
> # tc filter show dev eth0 parent ffff:
> filter pref 1 flower chain 0
> filter pref 1 flower chain 0 handle 0x1 classid ffff:2
> eth_type ipv4
> ip_proto udp
> dst_ip 192.168.1.1
> dst_port 22
> skip_sw
> in_hw
>
Much much better semantic. Thank you.
Have you tested many filter mapping to the same classid?
cheers,
jamal
^ permalink raw reply
* RE: [E] Re: [PATCH net-next 1/2] net sched act_vlan: Change stats update to use per-core stats
From: Kurup, Manish B @ 2017-10-11 12:35 UTC (permalink / raw)
To: Jamal Hadi Salim, Manish Kurup, xiyou.wangcong@gmail.com,
jiri@resnulli.us, davem@davemloft.net, netdev@vger.kernel.org
Cc: aring@mojatatu.com, mrv@mojatatu.com
In-Reply-To: <d79cf8b2-4c67-1101-0405-afec1e9f89c7@mojatatu.com>
Hi Jamal, Yes, that's an indentation mismatch - I put in one tab too many. Shall fix before commit.
Thanks,
-Manish
-----Original Message-----
From: Jamal Hadi Salim [mailto:jhs@mojatatu.com]
Sent: Wednesday, October 11, 2017 8:28 AM
To: Manish Kurup; xiyou.wangcong@gmail.com; jiri@resnulli.us; davem@davemloft.net; netdev@vger.kernel.org
Cc: aring@mojatatu.com; mrv@mojatatu.com; Kurup, Manish B
Subject: [E] Re: [PATCH net-next 1/2] net sched act_vlan: Change stats update to use per-core stats
minus lk
On 17-10-10 10:32 PM, Manish Kurup wrote:
> The VLAN action maintains one set of stats across all cores, and uses
> a spinlock to synchronize updates to it from the same. Changed this to
> use a per-CPU stats context instead.
> This change will result in better performance.
>
> Signed-off-by: Manish Kurup <manish.kurup@verizon.com>
> ---
> net/sched/act_vlan.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c index
> 16eb067..14c262c 100644
> --- a/net/sched/act_vlan.c
> +++ b/net/sched/act_vlan.c
> @@ -30,9 +30,10 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
> int err;
> u16 tci;
>
> - spin_lock(&v->tcf_lock);
> tcf_lastuse_update(&v->tcf_tm);
> - bstats_update(&v->tcf_bstats, skb);
> + bstats_cpu_update(this_cpu_ptr(v->common.cpu_bstats), skb);
> +
> + spin_lock(&v->tcf_lock);
> action = v->tcf_action;
>
> /* Ensure 'data' points at mac_header prior calling vlan
> manipulating @@ -85,7 +86,8 @@ static int tcf_vlan(struct sk_buff
> *skb, const struct tc_action *a,
>
> drop:
> action = TC_ACT_SHOT;
> - v->tcf_qstats.drops++;
> + qstats_drop_inc(this_cpu_ptr(v->common.cpu_qstats));
> +
> unlock:
> if (skb_at_tc_ingress(skb))
> skb_pull_rcsum(skb, skb->mac_len); @@ -172,7 +174,7 @@ static int
> tcf_vlan_init(struct net *net, struct nlattr *nla,
>
> if (!exists) {
> ret = tcf_idr_create(tn, parm->index, est, a,
> - &act_vlan_ops, bind, false);
> + &act_vlan_ops, bind, true);
>
Indentation mismatch here?
Otherwise looks good to me.
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
cheers,
jamal
^ permalink raw reply
* RE: [PATCH] rtl8xxxu: mark expected switch fall-throughs
From: David Laight @ 2017-10-11 12:54 UTC (permalink / raw)
To: 'Joe Perches', Gustavo A. R. Silva, Jes Sorensen,
Kalle Valo
Cc: linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Arnaldo Carvalho de Melo
In-Reply-To: <1507717237.3552.48.camel@perches.com>
From: Joe Perches
> Sent: 11 October 2017 11:21
> On Tue, 2017-10-10 at 14:30 -0500, Gustavo A. R. Silva wrote:
> > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> > where we are expecting to fall through.
>
> perhaps use Arnaldo's idea:
>
> https://lkml.org/lkml/2017/2/9/845
> https://lkml.org/lkml/2017/2/10/485
gah, that is even uglier and requires a chase through
headers to find out what it means.
David
^ permalink raw reply
* Re: [PATCH net-next v3 5/5] selinux: bpf: Add addtional check for bpf object file receive
From: Stephen Smalley @ 2017-10-11 12:54 UTC (permalink / raw)
To: Chenbo Feng, linux-security-module, netdev, SELinux
Cc: Daniel Borkmann, Chenbo Feng, Alexei Starovoitov, lorenzo
In-Reply-To: <20171011000930.133308-6-chenbofeng.kernel@gmail.com>
On Tue, 2017-10-10 at 17:09 -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/lsm_hooks.h | 17 ++++++++++
> include/linux/security.h | 9 ++++++
> kernel/bpf/syscall.c | 27 ++++++++++++++--
> security/security.c | 8 +++++
> security/selinux/hooks.c | 67
> +++++++++++++++++++++++++++++++++++++++
> security/selinux/include/objsec.h | 9 ++++++
> 6 files changed, 135 insertions(+), 2 deletions(-)
>
> 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..aee69e564c50 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -324,11 +324,22 @@ static const struct file_operations
> bpf_map_fops = {
>
> int bpf_map_new_fd(struct bpf_map *map, int flags)
> {
> + int fd;
> + struct fd f;
> if (security_bpf_map(map, OPEN_FMODE(flags)))
> return -EPERM;
>
> - return anon_inode_getfd("bpf-map", &bpf_map_fops, map,
> + fd = anon_inode_getfd("bpf-map", &bpf_map_fops, map,
> flags | O_CLOEXEC);
> + if (fd < 0)
> + return fd;
> +
> + f = fdget(fd);
> + if (!f.file)
> + return -EBADF;
This seems convoluted and unnecessarily inefficient, since
anon_inode_getfd() has the struct file and could have directly returned
it instead of having to go through fdget() on a fd we just installed.
Also, couldn't the fd->file mapping have changed underneath us between
fd_install() and fdget()?
I would think it would be safer and more efficient to create an
anon_inode_getfdandfile() or similar interface and use that, so that we
can just pass the file it set up to the hook. Obviously that would
need to be reviewed by the vfs folks.
> + security_bpf_map_file(map, f.file);
> + fdput(f);
> + return fd;
> }
>
> int bpf_get_file_flag(int flags)
> @@ -975,11 +986,23 @@ static const struct file_operations
> bpf_prog_fops = {
>
> int bpf_prog_new_fd(struct bpf_prog *prog)
> {
> + int fd;
> + struct fd f;
> +
> if (security_bpf_prog(prog))
> return -EPERM;
>
> - return anon_inode_getfd("bpf-prog", &bpf_prog_fops, prog,
> + fd = anon_inode_getfd("bpf-prog", &bpf_prog_fops, prog,
> O_RDWR | O_CLOEXEC);
> + if (fd < 0)
> + return fd;
> +
> + f = fdget(fd);
> + if (!f.file)
> + return -EBADF;
> + security_bpf_prog_file(prog->aux, f.file);
> + fdput(f);
> + return fd;
> }
>
> static struct bpf_prog *____bpf_prog_get(struct fd f)
> 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 94e473b9c884..0a6ef20513b0 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,14 @@ static int file_has_perm(const struct cred
> *cred,
> goto out;
> }
>
> +#ifdef CONFIG_BPF_SYSCALL
> + if (fsec->bpf_type) {
> + 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 +2177,14 @@ static int selinux_binder_transfer_file(struct
> task_struct *from,
> return rc;
> }
>
> +#ifdef CONFIG_BPF_SYSCALL
> + if (fsec->bpf_type) {
> + rc = bpf_file_check(file, sid);
> + if (rc)
> + return rc;
> + }
> +#endif
> +
> if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> return 0;
>
> @@ -6288,6 +6308,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 (fsec->bpf_type == BPF_MAP) {
> + ret = avc_has_perm(sid, fsec->bpf_sid, SECCLASS_BPF,
> + bpf_map_fmode_to_av(file-
> >f_mode), NULL);
> + if (ret)
> + return ret;
> + } else if (fsec->bpf_type == BPF_PROG) {
> + ret = avc_has_perm(sid, fsec->bpf_sid, SECCLASS_BPF,
> + 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 +6398,24 @@ 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->bpf_type = BPF_MAP;
> + fsec->bpf_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->bpf_type = BPF_PROG;
> + fsec->bpf_sid = bpfsec->sid;
> +}
> #endif
>
> static struct security_hook_list selinux_hooks[] __lsm_ro_after_init
> = {
> @@ -6581,6 +6646,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
> };
>
> diff --git a/security/selinux/include/objsec.h
> b/security/selinux/include/objsec.h
> index 3d54468ce334..0162648761f9 100644
> --- a/security/selinux/include/objsec.h
> +++ b/security/selinux/include/objsec.h
> @@ -67,11 +67,20 @@ struct inode_security_struct {
> spinlock_t lock;
> };
>
> +enum bpf_obj_type {
> + BPF_MAP = 1,
> + BPF_PROG,
> +};
> +
> struct file_security_struct {
> u32 sid; /* SID of open file description */
> u32 fown_sid; /* SID of file owner (for
> SIGIO) */
> u32 isid; /* SID of inode at the time of file
> open */
> u32 pseqno; /* Policy seqno at the time of
> file open */
> +#ifdef CONFIG_BPF_SYSCALL
> + unsigned char bpf_type;
> + u32 bpf_sid;
> +#endif
> };
Can you check how this impacts the size of the file_security_cache
objects, and thus the memory overhead imposed on all open files?
If it is significant, do we need to cache the bpf_sid here or could we
just store the bpf_type and then dereference the bpfsec if it is a map
or prog?
>
> struct superblock_security_struct {
^ permalink raw reply
* Re: [jkirsher/next-queue PATCH v4 0/6] tc-flower based cloud filters in i40e
From: Jiri Pirko @ 2017-10-11 12:56 UTC (permalink / raw)
To: Amritha Nambiar
Cc: intel-wired-lan, jeffrey.t.kirsher, alexander.h.duyck, netdev,
jhs, alexander.duyck, xiyou.wangcong
In-Reply-To: <150768099999.5320.1633617713417675266.stgit@anamdev.jf.intel.com>
Wed, Oct 11, 2017 at 02:24:12AM CEST, amritha.nambiar@intel.com wrote:
>This patch series enables configuring cloud filters in i40e
>using the tc-flower classifier. The classification function
>of the filter is to match a packet to a class. cls_flower is
>extended to offload classid to hardware. The offloaded classid
>is used direct matched packets to a traffic class on the device.
>The approach here is similar to the tc 'prio' qdisc which uses
>the classid for band selection. The ingress qdisc is called ffff:0,
>so traffic classes are ffff:1 to ffff:8 (i40e has max of 8 TCs).
NACK. This clearly looks like abuse of classid to something
else. Classid is here to identify qdisc instance. However, you use it
for hw tclass identification. This is mixing of apples and oranges.
Why?
Please don't try to abuse things! This is not nice.
>TC0 is minor number 1, TC1 is minor number 2 etc.
>
>The cloud filters are added for a VSI and are cleaned up when
>the VSI is deleted. The filters that match on L4 ports needs
>enhanced admin queue functions with big buffer support for
>extended fields in cloud filter commands.
>
>Example:
># tc qdisc add dev eth0 ingress
># ethtool -K eth0 hw-tc-offload on
>
>Match Dst IPv4,Dst Port and route to TC1:
># tc filter add dev eth0 protocol ip parent ffff: prio 1 flower\
> dst_ip 192.168.1.1/32 ip_proto udp dst_port 22\
> skip_sw classid ffff:2
>
># tc filter show dev eth0 parent ffff:
>filter pref 1 flower chain 0
>filter pref 1 flower chain 0 handle 0x1 classid ffff:2
> eth_type ipv4
> ip_proto udp
> dst_ip 192.168.1.1
> dst_port 22
> skip_sw
> in_hw
>
>v4: classid based approach to set traffic class for matched packets.
>
>Authors:
>Amritha Nambiar <amritha.nambiar@intel.com>
>Kiran Patil <kiran.patil@intel.com>
>Anjali Singhai Jain <anjali.singhai@intel.com>
>Jingjing Wu <jingjing.wu@intel.com>
>---
>
>Amritha Nambiar (6):
> cls_flower: Offload classid to hardware
> i40e: Map TCs with the VSI seids
> i40e: Cloud filter mode for set_switch_config command
> i40e: Admin queue definitions for cloud filters
> i40e: Clean up of cloud filters
> i40e: Enable cloud filters via tc-flower
>
>
> drivers/net/ethernet/intel/i40e/i40e.h | 55 +
> drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h | 143 +++
> drivers/net/ethernet/intel/i40e/i40e_common.c | 193 ++++
> drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 2
> drivers/net/ethernet/intel/i40e/i40e_main.c | 941 +++++++++++++++++++-
> drivers/net/ethernet/intel/i40e/i40e_prototype.h | 18
> drivers/net/ethernet/intel/i40e/i40e_type.h | 10
> .../net/ethernet/intel/i40evf/i40e_adminq_cmd.h | 113 ++
> include/net/pkt_cls.h | 1
> net/sched/cls_flower.c | 2
> 10 files changed, 1439 insertions(+), 39 deletions(-)
>
>--
^ permalink raw reply
* Re: [E] Re: [PATCH net-next 1/2] net sched act_vlan: Change stats update to use per-core stats
From: Jiri Pirko @ 2017-10-11 12:58 UTC (permalink / raw)
To: Kurup, Manish B
Cc: Jamal Hadi Salim, Manish Kurup, xiyou.wangcong@gmail.com,
davem@davemloft.net, netdev@vger.kernel.org, aring@mojatatu.com,
mrv@mojatatu.com
In-Reply-To: <10f80e225811433d89d58f139fba8026@OMZP1LUMXCA01.uswin.ad.vzwcorp.com>
Wed, Oct 11, 2017 at 02:35:50PM CEST, manish.kurup@verizon.com wrote:
>Hi Jamal, Yes, that's an indentation mismatch - I put in one tab too many. Shall fix before commit.
No top-posting please!
>
>Thanks,
>
>-Manish
>
>-----Original Message-----
>From: Jamal Hadi Salim [mailto:jhs@mojatatu.com]
>Sent: Wednesday, October 11, 2017 8:28 AM
>To: Manish Kurup; xiyou.wangcong@gmail.com; jiri@resnulli.us; davem@davemloft.net; netdev@vger.kernel.org
>Cc: aring@mojatatu.com; mrv@mojatatu.com; Kurup, Manish B
>Subject: [E] Re: [PATCH net-next 1/2] net sched act_vlan: Change stats update to use per-core stats
>
>minus lk
>
>On 17-10-10 10:32 PM, Manish Kurup wrote:
>> The VLAN action maintains one set of stats across all cores, and uses
>> a spinlock to synchronize updates to it from the same. Changed this to
>> use a per-CPU stats context instead.
>> This change will result in better performance.
>>
>> Signed-off-by: Manish Kurup <manish.kurup@verizon.com>
>> ---
>> net/sched/act_vlan.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c index
>> 16eb067..14c262c 100644
>> --- a/net/sched/act_vlan.c
>> +++ b/net/sched/act_vlan.c
>> @@ -30,9 +30,10 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
>> int err;
>> u16 tci;
>>
>> - spin_lock(&v->tcf_lock);
>> tcf_lastuse_update(&v->tcf_tm);
>> - bstats_update(&v->tcf_bstats, skb);
>> + bstats_cpu_update(this_cpu_ptr(v->common.cpu_bstats), skb);
>> +
>> + spin_lock(&v->tcf_lock);
>> action = v->tcf_action;
>>
>> /* Ensure 'data' points at mac_header prior calling vlan
>> manipulating @@ -85,7 +86,8 @@ static int tcf_vlan(struct sk_buff
>> *skb, const struct tc_action *a,
>>
>> drop:
>> action = TC_ACT_SHOT;
>> - v->tcf_qstats.drops++;
>> + qstats_drop_inc(this_cpu_ptr(v->common.cpu_qstats));
>> +
>> unlock:
>> if (skb_at_tc_ingress(skb))
>> skb_pull_rcsum(skb, skb->mac_len); @@ -172,7 +174,7 @@ static int
>> tcf_vlan_init(struct net *net, struct nlattr *nla,
>>
>> if (!exists) {
>> ret = tcf_idr_create(tn, parm->index, est, a,
>> - &act_vlan_ops, bind, false);
>> + &act_vlan_ops, bind, true);
>>
>
>Indentation mismatch here?
>
>Otherwise looks good to me.
>
>Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
>cheers,
>jamal
^ permalink raw reply
* Re: [PATCH net-next v2 4/5] selinux: bpf: Add selinux check for eBPF syscall operations
From: Stephen Smalley @ 2017-10-11 13:00 UTC (permalink / raw)
To: Chenbo Feng
Cc: Daniel Borkmann, netdev, Chenbo Feng, linux-security-module,
SELinux, Alexei Starovoitov, Lorenzo Colitti
In-Reply-To: <CAMOXUJmKtpB6LU2V3AyBvMFizYDCrZGQ5ZG3+z=5FOyooQGy=w@mail.gmail.com>
On Tue, 2017-10-10 at 10:54 -0700, Chenbo Feng via Selinux wrote:
> 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.
If we anticipated a large number of permissions being defined for
either bpf maps or programs, or if we were labeling them differently
(e.g. inheriting from creator for one, while using type transitions for
another), then it might make more sense to split the class (so that we
can support up to 32 distinct permissions for each one, or because we'd
never end up allowing both map and prog permissions to the same target
SID/context). But in this case I can't see any benefit to using two
classes, and it would consume more memory to do so.
BTW, please be sure to cc Paul Moore on these patches if you aren't
already since he is the selinux kernel tree maintainer.
> > 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
* Re: [RFC net-next 1/4] net: ipv6: Make inet6addr_validator a blocking notifier
From: Ido Schimmel @ 2017-10-11 13:08 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, jiri, idosch, kjlx
In-Reply-To: <1507653665-20540-2-git-send-email-dsahern@gmail.com>
On Tue, Oct 10, 2017 at 09:41:02AM -0700, David Ahern wrote:
> inet6addr_validator chain was added by commit 3ad7d2468f79f ("Ipvlan
> should return an error when an address is already in use") to allow
> address validation before changes are committed and to be able to
> fail the address change with an error back to the user. The address
> validation is not done for addresses received from router
> advertisements.
>
> Handling RAs in softirq context is the only reason for the notifier
> chain to be atomic versus blocking. Since the only current user, ipvlan,
> of the validator chain ignores softirq context, the notifier can be made
> blocking and simply not invoked for softirq path.
>
> The blocking option is needed by spectrum for example to validate
> resources for an adding an address to an interface.
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
With the fixup posted later:
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
BTW, the in_softirq() check in ipvlan_addr6_validator_event() can be
removed after this patch.
^ permalink raw reply
* Re: [PATCH net-next 2/2] net sched act_vlan: VLAN action rewrite to use RCU lock/unlock and update
From: Eric Dumazet @ 2017-10-11 13:10 UTC (permalink / raw)
To: Manish Kurup
Cc: jhs, xiyou.wangcong, jiri, davem, netdev, linux-kernel, aring,
mrv, manish.kurup
In-Reply-To: <1507689219-22993-1-git-send-email-manish.kurup@verizon.com>
On Tue, 2017-10-10 at 22:33 -0400, Manish Kurup wrote:
> Using a spinlock in the VLAN action causes performance issues when the VLAN
> action is used on multiple cores. Rewrote the VLAN action to use RCU read
> locking for reads and updates instead.
>
> Signed-off-by: Manish Kurup <manish.kurup@verizon.com>
> ---
> include/net/tc_act/tc_vlan.h | 21 ++++++++-----
> net/sched/act_vlan.c | 73 ++++++++++++++++++++++++++++++--------------
> 2 files changed, 63 insertions(+), 31 deletions(-)
>
> diff --git a/include/net/tc_act/tc_vlan.h b/include/net/tc_act/tc_vlan.h
> index c2090df..67fd355 100644
> --- a/include/net/tc_act/tc_vlan.h
> +++ b/include/net/tc_act/tc_vlan.h
> @@ -13,12 +13,17 @@
> #include <net/act_api.h>
> #include <linux/tc_act/tc_vlan.h>
>
> +struct tcf_vlan_params {
> + struct rcu_head rcu;
> + int tcfv_action;
> + u16 tcfv_push_vid;
> + __be16 tcfv_push_proto;
> + u8 tcfv_push_prio;
> +};
> +
> struct tcf_vlan {
> struct tc_action common;
> - int tcfv_action;
> - u16 tcfv_push_vid;
> - __be16 tcfv_push_proto;
> - u8 tcfv_push_prio;
> + struct tcf_vlan_params __rcu *vlan_p;
> };
> #define to_vlan(a) ((struct tcf_vlan *)a)
>
> @@ -33,22 +38,22 @@ static inline bool is_tcf_vlan(const struct tc_action *a)
>
> static inline u32 tcf_vlan_action(const struct tc_action *a)
> {
> - return to_vlan(a)->tcfv_action;
> + return to_vlan(a)->vlan_p->tcfv_action;
This is not proper RCU : ->vlan_p should be read once, and using
rcu_dereference()
So the caller should provide to this helper the 'p' pointer instead of
'a'
CONFIG_SPARSE_RCU_POINTER=y
and make C=2 would probably give you warnings about that.
^ permalink raw reply
* Re: [PATCH net-next 2/2] net sched act_vlan: VLAN action rewrite to use RCU lock/unlock and update
From: Jiri Pirko @ 2017-10-11 13:13 UTC (permalink / raw)
To: Manish Kurup
Cc: jhs, xiyou.wangcong, davem, netdev, linux-kernel, aring, mrv,
manish.kurup
In-Reply-To: <1507689219-22993-1-git-send-email-manish.kurup@verizon.com>
Wed, Oct 11, 2017 at 04:33:39AM CEST, kurup.manish@gmail.com wrote:
>Using a spinlock in the VLAN action causes performance issues when the VLAN
>action is used on multiple cores. Rewrote the VLAN action to use RCU read
>locking for reads and updates instead.
>
>Signed-off-by: Manish Kurup <manish.kurup@verizon.com>
>---
> include/net/tc_act/tc_vlan.h | 21 ++++++++-----
> net/sched/act_vlan.c | 73 ++++++++++++++++++++++++++++++--------------
> 2 files changed, 63 insertions(+), 31 deletions(-)
>
>diff --git a/include/net/tc_act/tc_vlan.h b/include/net/tc_act/tc_vlan.h
>index c2090df..67fd355 100644
>--- a/include/net/tc_act/tc_vlan.h
>+++ b/include/net/tc_act/tc_vlan.h
>@@ -13,12 +13,17 @@
> #include <net/act_api.h>
> #include <linux/tc_act/tc_vlan.h>
>
>+struct tcf_vlan_params {
>+ struct rcu_head rcu;
Usually rcu_head is put at the very end of struct.
>+ int tcfv_action;
>+ u16 tcfv_push_vid;
>+ __be16 tcfv_push_proto;
>+ u8 tcfv_push_prio;
>+};
>+
> struct tcf_vlan {
> struct tc_action common;
>- int tcfv_action;
>- u16 tcfv_push_vid;
>- __be16 tcfv_push_proto;
>- u8 tcfv_push_prio;
>+ struct tcf_vlan_params __rcu *vlan_p;
> };
> #define to_vlan(a) ((struct tcf_vlan *)a)
>
>@@ -33,22 +38,22 @@ static inline bool is_tcf_vlan(const struct tc_action *a)
>
> static inline u32 tcf_vlan_action(const struct tc_action *a)
> {
>- return to_vlan(a)->tcfv_action;
>+ return to_vlan(a)->vlan_p->tcfv_action;
> }
>
> static inline u16 tcf_vlan_push_vid(const struct tc_action *a)
> {
>- return to_vlan(a)->tcfv_push_vid;
>+ return to_vlan(a)->vlan_p->tcfv_push_vid;
> }
>
> static inline __be16 tcf_vlan_push_proto(const struct tc_action *a)
> {
>- return to_vlan(a)->tcfv_push_proto;
>+ return to_vlan(a)->vlan_p->tcfv_push_proto;
> }
>
> static inline u8 tcf_vlan_push_prio(const struct tc_action *a)
> {
>- return to_vlan(a)->tcfv_push_prio;
>+ return to_vlan(a)->vlan_p->tcfv_push_prio;
All these helpers should use rtnl_dereference() as well
> }
>
> #endif /* __NET_TC_VLAN_H */
>diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
>index 14c262c..9bb0236 100644
>--- a/net/sched/act_vlan.c
>+++ b/net/sched/act_vlan.c
>@@ -29,31 +29,37 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
> int action;
> int err;
> u16 tci;
>+ struct tcf_vlan_params *p;
>
> tcf_lastuse_update(&v->tcf_tm);
> bstats_cpu_update(this_cpu_ptr(v->common.cpu_bstats), skb);
>
>- spin_lock(&v->tcf_lock);
>- action = v->tcf_action;
>-
> /* Ensure 'data' points at mac_header prior calling vlan manipulating
> * functions.
> */
> if (skb_at_tc_ingress(skb))
> skb_push_rcsum(skb, skb->mac_len);
>
>- switch (v->tcfv_action) {
>+ rcu_read_lock();
>+
>+ action = READ_ONCE(v->tcf_action);
>+
Too many empty lines. At least remove the one above this ^
>+ p = rcu_dereference(v->vlan_p);
>+
>+ switch (p->tcfv_action) {
> case TCA_VLAN_ACT_POP:
> err = skb_vlan_pop(skb);
> if (err)
> goto drop;
> break;
>+
> case TCA_VLAN_ACT_PUSH:
>- err = skb_vlan_push(skb, v->tcfv_push_proto, v->tcfv_push_vid |
>- (v->tcfv_push_prio << VLAN_PRIO_SHIFT));
>+ err = skb_vlan_push(skb, p->tcfv_push_proto, p->tcfv_push_vid |
>+ (p->tcfv_push_prio << VLAN_PRIO_SHIFT));
Again, indentation is odd. Please fix.
> if (err)
> goto drop;
> break;
>+
Avoid adding this unrelated empty line.
> case TCA_VLAN_ACT_MODIFY:
> /* No-op if no vlan tag (either hw-accel or in-payload) */
> if (!skb_vlan_tagged(skb))
>@@ -69,15 +75,16 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
> goto drop;
> }
> /* replace the vid */
>- tci = (tci & ~VLAN_VID_MASK) | v->tcfv_push_vid;
>+ tci = (tci & ~VLAN_VID_MASK) | p->tcfv_push_vid;
> /* replace prio bits, if tcfv_push_prio specified */
>- if (v->tcfv_push_prio) {
>+ if (p->tcfv_push_prio) {
> tci &= ~VLAN_PRIO_MASK;
>- tci |= v->tcfv_push_prio << VLAN_PRIO_SHIFT;
>+ tci |= p->tcfv_push_prio << VLAN_PRIO_SHIFT;
> }
> /* put updated tci as hwaccel tag */
>- __vlan_hwaccel_put_tag(skb, v->tcfv_push_proto, tci);
>+ __vlan_hwaccel_put_tag(skb, p->tcfv_push_proto, tci);
> break;
>+
Again, avoid adding this unrelated empty line.
> default:
> BUG();
> }
>@@ -89,6 +96,7 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
> qstats_drop_inc(this_cpu_ptr(v->common.cpu_qstats));
>
> unlock:
>+ rcu_read_unlock();
> if (skb_at_tc_ingress(skb))
> skb_pull_rcsum(skb, skb->mac_len);
>
>@@ -111,6 +119,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
> struct nlattr *tb[TCA_VLAN_MAX + 1];
> struct tc_vlan *parm;
> struct tcf_vlan *v;
>+ struct tcf_vlan_params *p, *p_old;
The famous reverse-christmas-tree rule dictates this to be put right
above "struct tc_vlan *parm" :)
> int action;
> __be16 push_vid = 0;
> __be16 push_proto = 0;
>@@ -187,16 +196,33 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
>
> v = to_vlan(*a);
>
>- spin_lock_bh(&v->tcf_lock);
>-
>- v->tcfv_action = action;
>- v->tcfv_push_vid = push_vid;
>- v->tcfv_push_prio = push_prio;
>- v->tcfv_push_proto = push_proto;
>+ ASSERT_RTNL();
>+ p = kzalloc(sizeof(*p), GFP_KERNEL);
>+ if (unlikely(!p)) {
>+ if (ovr)
>+ tcf_idr_release(*a, bind);
>+ return -ENOMEM;
>+ }
>
> v->tcf_action = parm->action;
>
>- spin_unlock_bh(&v->tcf_lock);
>+ p_old = rtnl_dereference(v->vlan_p);
>+
>+ if (ovr)
>+ spin_lock_bh(&v->tcf_lock);
>+
>+ p->tcfv_action = action;
>+ p->tcfv_push_vid = push_vid;
>+ p->tcfv_push_prio = push_prio;
>+ p->tcfv_push_proto = push_proto;
>+
>+ rcu_assign_pointer(v->vlan_p, p);
>+
>+ if (ovr)
>+ spin_unlock_bh(&v->tcf_lock);
>+
>+ if (p_old)
>+ kfree_rcu(p_old, rcu);
>
> if (ret == ACT_P_CREATED)
> tcf_idr_insert(tn, *a);
>@@ -208,25 +234,26 @@ static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
> {
> unsigned char *b = skb_tail_pointer(skb);
> struct tcf_vlan *v = to_vlan(a);
>+ struct tcf_vlan_params *p = rtnl_dereference(v->vlan_p);
> struct tc_vlan opt = {
> .index = v->tcf_index,
> .refcnt = v->tcf_refcnt - ref,
> .bindcnt = v->tcf_bindcnt - bind,
> .action = v->tcf_action,
>- .v_action = v->tcfv_action,
>+ .v_action = p->tcfv_action,
> };
> struct tcf_t t;
>
> if (nla_put(skb, TCA_VLAN_PARMS, sizeof(opt), &opt))
> goto nla_put_failure;
>
>- if ((v->tcfv_action == TCA_VLAN_ACT_PUSH ||
>- v->tcfv_action == TCA_VLAN_ACT_MODIFY) &&
>- (nla_put_u16(skb, TCA_VLAN_PUSH_VLAN_ID, v->tcfv_push_vid) ||
>+ if ((p->tcfv_action == TCA_VLAN_ACT_PUSH ||
>+ p->tcfv_action == TCA_VLAN_ACT_MODIFY) &&
>+ (nla_put_u16(skb, TCA_VLAN_PUSH_VLAN_ID, p->tcfv_push_vid) ||
> nla_put_be16(skb, TCA_VLAN_PUSH_VLAN_PROTOCOL,
>- v->tcfv_push_proto) ||
>+ p->tcfv_push_proto) ||
> (nla_put_u8(skb, TCA_VLAN_PUSH_VLAN_PRIORITY,
>- v->tcfv_push_prio))))
>+ p->tcfv_push_prio))))
> goto nla_put_failure;
>
> tcf_tm_dump(&t, &v->tcf_tm);
>--
>2.7.4
>
Besides the couple of nits I pointed out, this looks nice. Thanks!
^ permalink raw reply
* Re: [PATCH net-next 1/2] net sched act_vlan: Change stats update to use per-core stats
From: Jiri Pirko @ 2017-10-11 13:15 UTC (permalink / raw)
To: Manish Kurup
Cc: jhs, xiyou.wangcong, davem, netdev, linux-kernel, aring, mrv,
manish.kurup
In-Reply-To: <1507689147-22941-1-git-send-email-manish.kurup@verizon.com>
Wed, Oct 11, 2017 at 04:32:27AM CEST, kurup.manish@gmail.com wrote:
>The VLAN action maintains one set of stats across all cores, and uses a
>spinlock to synchronize updates to it from the same. Changed this to use a
>per-CPU stats context instead.
>This change will result in better performance.
>
>Signed-off-by: Manish Kurup <manish.kurup@verizon.com>
>---
> net/sched/act_vlan.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
>diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
>index 16eb067..14c262c 100644
>--- a/net/sched/act_vlan.c
>+++ b/net/sched/act_vlan.c
>@@ -30,9 +30,10 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
> int err;
> u16 tci;
>
>- spin_lock(&v->tcf_lock);
> tcf_lastuse_update(&v->tcf_tm);
>- bstats_update(&v->tcf_bstats, skb);
>+ bstats_cpu_update(this_cpu_ptr(v->common.cpu_bstats), skb);
>+
>+ spin_lock(&v->tcf_lock);
> action = v->tcf_action;
>
> /* Ensure 'data' points at mac_header prior calling vlan manipulating
>@@ -85,7 +86,8 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
>
> drop:
> action = TC_ACT_SHOT;
>- v->tcf_qstats.drops++;
>+ qstats_drop_inc(this_cpu_ptr(v->common.cpu_qstats));
>+
> unlock:
> if (skb_at_tc_ingress(skb))
> skb_pull_rcsum(skb, skb->mac_len);
>@@ -172,7 +174,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
>
> if (!exists) {
> ret = tcf_idr_create(tn, parm->index, est, a,
>- &act_vlan_ops, bind, false);
>+ &act_vlan_ops, bind, true);
Please fix this indent nit as pointed out by Jamal.
Feel free to add my tag:
Acked-by: Jiri Pirko <jiri@mellanox.com>
Thanks!
^ permalink raw reply
* Re: [PATCH v2 nf-next 1/2] netfilter: x_tables: make xt_replace_table wait until old rules are not used anymore
From: Eric Dumazet @ 2017-10-11 13:23 UTC (permalink / raw)
To: Florian Westphal; +Cc: netdev, Dan Williams
In-Reply-To: <20171010213945.19074-2-fw@strlen.de>
On Tue, Oct 10, 2017 at 2:39 PM, Florian Westphal <fw@strlen.de> wrote:
> xt_replace_table relies on table replacement counter retrieval (which
> uses xt_recseq to synchronize pcpu counters).
>
> This is fine, however with large rule set get_counters() can take
> a very long time -- it needs to synchronize all counters because
> it has to assume concurrent modifications can occur.
>
> Make xt_replace_table synchronize by itself by waiting until all cpus
> had an even seqcount.
>
> This allows a followup patch to copy the counters of the old ruleset
> without any synchonization after xt_replace_table has completed.
>
> Cc: Dan Williams <dcbw@redhat.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
> v2: fix Erics email address
>
> net/netfilter/x_tables.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
> index c83a3b5e1c6c..f2d4a365768f 100644
> --- a/net/netfilter/x_tables.c
> +++ b/net/netfilter/x_tables.c
> @@ -1153,6 +1153,7 @@ xt_replace_table(struct xt_table *table,
> int *error)
> {
> struct xt_table_info *private;
> + unsigned int cpu;
> int ret;
>
> ret = xt_jumpstack_alloc(newinfo);
> @@ -1184,12 +1185,20 @@ xt_replace_table(struct xt_table *table,
>
> /*
> * Even though table entries have now been swapped, other CPU's
> - * may still be using the old entries. This is okay, because
> - * resynchronization happens because of the locking done
> - * during the get_counters() routine.
> + * may still be using the old entries...
> */
> local_bh_enable();
>
> + /* ... so wait for even xt_recseq on all cpus */
> + for_each_possible_cpu(cpu) {
> + seqcount_t *s = &per_cpu(xt_recseq, cpu);
> +
> + while (raw_read_seqcount(s) & 1)
> + cpu_relax();
> +
> + cond_resched();
> + }
It seems that we could also check :
1) If low order bit of sequence is 0
Or
2) the value has changed
for_each_possible_cpu(cpu) {
seqcount_t *s = &per_cpu(xt_recseq, cpu);
u32 seq = raw_read_seqcount(s) ;
if (seq & 1) {
do {
cpu_relax();
} while (seq == raw_read_seqcount(s));
Thanks !
^ 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