* Re: [PATCH] bpf: Add Python 3 support to selftests scripts for bpf
From: Daniel Borkmann @ 2018-07-20 20:45 UTC (permalink / raw)
To: Jeremy Cline, Alexei Starovoitov, Shuah Khan
Cc: netdev, linux-kernel, linux-kselftest, Lawrence Brakmo,
jakub.kicinski
In-Reply-To: <20180718213610.19618-1-jcline@redhat.com>
On 07/18/2018 11:36 PM, Jeremy Cline wrote:
> Adjust tcp_client.py and tcp_server.py to work with Python 3 by using
> the print function, marking string literals as bytes, and using the
> newer exception syntax. This should be functionally equivalent and
> support Python 2.6 through Python 3.7.
>
> Signed-off-by: Jeremy Cline <jcline@redhat.com>
Thanks for the patch, Jeremy! Given we also have test_offload.py in BPF
kselftests and it is written for python 3 only, it would probably make
sense to adapt the tcp_{client,server}.py towards python 3 as well, so
we wouldn't need to keep extra compat for 2 and have a consistent version
dependency. Lawrence / Jeremy, any objections?
> tools/testing/selftests/bpf/tcp_client.py | 12 ++++++------
> tools/testing/selftests/bpf/tcp_server.py | 17 +++++++++--------
> 2 files changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/tcp_client.py b/tools/testing/selftests/bpf/tcp_client.py
> index 481dccdf140c..9fe5f1b5c020 100755
> --- a/tools/testing/selftests/bpf/tcp_client.py
> +++ b/tools/testing/selftests/bpf/tcp_client.py
> @@ -1,4 +1,4 @@
> -#!/usr/bin/env python2
> +#!/usr/bin/env python
> #
> # SPDX-License-Identifier: GPL-2.0
> #
> @@ -9,11 +9,11 @@ import subprocess
> import select
>
> def read(sock, n):
> - buf = ''
> + buf = b''
> while len(buf) < n:
> rem = n - len(buf)
> try: s = sock.recv(rem)
> - except (socket.error), e: return ''
> + except (socket.error) as e: return b''
> buf += s
> return buf
>
> @@ -22,7 +22,7 @@ def send(sock, s):
> count = 0
> while count < total:
> try: n = sock.send(s)
> - except (socket.error), e: n = 0
> + except (socket.error) as e: n = 0
> if n == 0:
> return count;
> count += n
> @@ -39,10 +39,10 @@ try:
> except socket.error as e:
> sys.exit(1)
>
> -buf = ''
> +buf = b''
> n = 0
> while n < 1000:
> - buf += '+'
> + buf += b'+'
> n += 1
>
> sock.settimeout(1);
> diff --git a/tools/testing/selftests/bpf/tcp_server.py b/tools/testing/selftests/bpf/tcp_server.py
> index bc454d7d0be2..1d4a40a6584b 100755
> --- a/tools/testing/selftests/bpf/tcp_server.py
> +++ b/tools/testing/selftests/bpf/tcp_server.py
> @@ -1,7 +1,8 @@
> -#!/usr/bin/env python2
> +#!/usr/bin/env python
> #
> # SPDX-License-Identifier: GPL-2.0
> #
> +from __future__ import print_function
>
> import sys, os, os.path, getopt
> import socket, time
> @@ -9,11 +10,11 @@ import subprocess
> import select
>
> def read(sock, n):
> - buf = ''
> + buf = b''
> while len(buf) < n:
> rem = n - len(buf)
> try: s = sock.recv(rem)
> - except (socket.error), e: return ''
> + except (socket.error) as e: return b''
> buf += s
> return buf
>
> @@ -22,7 +23,7 @@ def send(sock, s):
> count = 0
> while count < total:
> try: n = sock.send(s)
> - except (socket.error), e: n = 0
> + except (socket.error) as e: n = 0
> if n == 0:
> return count;
> count += n
> @@ -43,7 +44,7 @@ host = socket.gethostname()
>
> try: serverSocket.bind((host, 0))
> except socket.error as msg:
> - print 'bind fails: ', msg
> + print('bind fails: ' + str(msg))
>
> sn = serverSocket.getsockname()
> serverPort = sn[1]
> @@ -51,10 +52,10 @@ serverPort = sn[1]
> cmdStr = ("./tcp_client.py %d &") % (serverPort)
> os.system(cmdStr)
>
> -buf = ''
> +buf = b''
> n = 0
> while n < 500:
> - buf += '.'
> + buf += b'.'
> n += 1
>
> serverSocket.listen(MAX_PORTS)
> @@ -79,5 +80,5 @@ while True:
> serverSocket.close()
> sys.exit(0)
> else:
> - print 'Select timeout!'
> + print('Select timeout!')
> sys.exit(1)
>
^ permalink raw reply
* Re: [PATCH bpf] xdp: add NULL pointer check in __xdp_return()
From: Jakub Kicinski @ 2018-07-20 20:05 UTC (permalink / raw)
To: daniel, bjorn.topel, brouer
Cc: Martin KaFai Lau, Taehee Yoo, ast, netdev, Karlsson, Magnus
In-Reply-To: <20180720171821.ivtpo6obzx2v737c@kafai-mbp.dhcp.thefacebook.com>
On Fri, 20 Jul 2018 10:18:21 -0700, Martin KaFai Lau wrote:
> On Sat, Jul 21, 2018 at 01:04:45AM +0900, Taehee Yoo wrote:
> > rhashtable_lookup() can return NULL. so that NULL pointer
> > check routine should be added.
> >
> > Fixes: 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
> > Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> > ---
> > net/core/xdp.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > index 9d1f220..1c12bc7 100644
> > --- a/net/core/xdp.c
> > +++ b/net/core/xdp.c
> > @@ -345,7 +345,8 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
> > rcu_read_lock();
> > /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
> > xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
> > - xa->zc_alloc->free(xa->zc_alloc, handle);
> > + if (xa)
> > + xa->zc_alloc->free(xa->zc_alloc, handle);
> hmm...It is not clear to me the "!xa" case don't have to be handled?
Actually I have a more fundamental question about this interface I've
been meaning to ask.
IIUC free() can happen on any CPU at any time, when whatever device,
socket or CPU this got redirected to completed the TX. IOW there may
be multiple producers. Drivers would need to create spin lock a'la the
a9744f7ca200 ("xsk: fix potential race in SKB TX completion code") fix?
We need some form of internal kernel circulation which would be MPSC.
I'm currently hacking up the XSK code to tell me whether the frame was
consumed by the correct XSK, and always clone the frame otherwise
(claiming to be the "traditional" MEM_TYPE_PAGE_ORDER0).
I feel like I'm missing something about the code. Is redirect of
ZC/UMEM frame outside the xsk not possible and the only returns we will
see are from net/xdp/xsk.c? That would work, but I don't see such a
check. Help would be appreciated.
Also the fact that XSK bufs can't be freed, only completed, adds to the
pain of implementing AF_XDP, we'd certainly need some form of "give
back the frame, but I may need it later" SPSC mechanism, otherwise
driver writers will have tough time. Unless, again, I'm missing
something about the code :)
> > rcu_read_unlock();
> > default:
> > /* Not possible, checked in xdp_rxq_info_reg_mem_model() */
^ permalink raw reply
* Re: [PATCH] rfkill: fix spelling mistake contidion to condition
From: Richard Guy Briggs @ 2018-07-20 20:14 UTC (permalink / raw)
To: netdev; +Cc: Linux-Audit Mailing List
In-Reply-To: <6118147ea27ab5831956cf9dcad15b241c791420.1532035500.git.rgb@redhat.com>
On 2018-07-20 11:35, Richard Guy Briggs wrote:
> This came about while trying to determine if there would be any pattern
> match on contid, a new audit container identifier internal variable.
> Thsi was the only one.
Yikes, my own typing needs a check... and I notice another spelling
mistake in the following line "contition".
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> net/rfkill/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/rfkill/core.c b/net/rfkill/core.c
> index 59d0eb9..e89a009 100644
> --- a/net/rfkill/core.c
> +++ b/net/rfkill/core.c
> @@ -494,7 +494,7 @@ void rfkill_remove_epo_lock(void)
> /**
> * rfkill_is_epo_lock_active - returns true EPO is active
> *
> - * Returns 0 (false) if there is NOT an active EPO contidion,
> + * Returns 0 (false) if there is NOT an active EPO condition,
> * and 1 (true) if there is an active EPO contition, which
> * locks all radios in one of the BLOCKED states.
> *
> --
> 1.8.3.1
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
- RGB
^ permalink raw reply
* [PATCH net 0/4] vxlan: fix default fdb entry user-space notify ordering/race
From: Roopa Prabhu @ 2018-07-20 20:21 UTC (permalink / raw)
To: davem; +Cc: netdev
From: Roopa Prabhu <roopa@cumulusnetworks.com>
Problem:
In vxlan_newlink, a default fdb entry is added before register_netdev.
The default fdb creation function notifies user-space of the
fdb entry on the vxlan device which user-space does not know about yet.
(RTM_NEWNEIGH goes before RTM_NEWLINK for the same ifindex).
This series fixes the user-space netlink notification ordering issue
with the following changes:
- decouple fdb notify from fdb create.
- Move fdb notify after register_netdev.
- modify rtnl_configure_link to allow configuring a link early.
- Call rtnl_configure_link in vxlan newlink handler to notify
userspace about the newlink before fdb notify and
hence avoiding the user-space race.
Fixes: afbd8bae9c79 ("vxlan: add implicit fdb entry for default destination")
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
Roopa Prabhu (4):
vxlan: add new fdb alloc and create helpers
vxlan: make netlink notify in vxlan_fdb_destroy optional
rtnetlink: add rtnl_link_state check in rtnl_configure_link
vxlan: fix default fdb netlink notify ordering during netdev create
drivers/net/vxlan.c | 130 ++++++++++++++++++++++++++++++++++-----------------
net/core/rtnetlink.c | 9 ++--
2 files changed, 94 insertions(+), 45 deletions(-)
--
Dave, I am sending this series against net on your request on my net-next series.
https://marc.info/?l=linux-netdev&m=153098151929102&w=2
I did not know how long to wait before i send it to net. So sending it now.
As you also noted it is a very old bug. If you have changed your mind about stable or
if this is too early for net and you think we should soak it more in net-next,
pls feel free to drop. also, i will be happy to do a stable backport if needed. thanks.
^ permalink raw reply
* [PATCH net 1/4] rtnetlink: add rtnl_link_state check in rtnl_configure_link
From: Roopa Prabhu @ 2018-07-20 20:21 UTC (permalink / raw)
To: davem; +Cc: netdev
In-Reply-To: <1532118064-45513-1-git-send-email-roopa@cumulusnetworks.com>
From: Roopa Prabhu <roopa@cumulusnetworks.com>
rtnl_configure_link sets dev->rtnl_link_state to
RTNL_LINK_INITIALIZED and unconditionally calls
__dev_notify_flags to notify user-space of dev flags.
current call sequence for rtnl_configure_link
rtnetlink_newlink
rtnl_link_ops->newlink
rtnl_configure_link (unconditionally notifies userspace of
default and new dev flags)
If a newlink handler wants to call rtnl_configure_link
early, we will end up with duplicate notifications to
user-space.
This patch fixes rtnl_configure_link to check rtnl_link_state
and call __dev_notify_flags with gchanges = 0 if already
RTNL_LINK_INITIALIZED.
Later in the series, this patch will help the following sequence
where a driver implementing newlink can call rtnl_configure_link
to initialize the link early.
makes the following call sequence work:
rtnetlink_newlink
rtnl_link_ops->newlink (vxlan) -> rtnl_configure_link (initializes
link and notifies
user-space of default
dev flags)
rtnl_configure_link (updates dev flags if requested by user ifm
and notifies user-space of new dev flags)
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
net/core/rtnetlink.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 5ef6122..e3f743c 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2759,9 +2759,12 @@ int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm)
return err;
}
- dev->rtnl_link_state = RTNL_LINK_INITIALIZED;
-
- __dev_notify_flags(dev, old_flags, ~0U);
+ if (dev->rtnl_link_state == RTNL_LINK_INITIALIZED) {
+ __dev_notify_flags(dev, old_flags, 0U);
+ } else {
+ dev->rtnl_link_state = RTNL_LINK_INITIALIZED;
+ __dev_notify_flags(dev, old_flags, ~0U);
+ }
return 0;
}
EXPORT_SYMBOL(rtnl_configure_link);
--
2.1.4
^ permalink raw reply related
* [PATCH net 2/4] vxlan: add new fdb alloc and create helpers
From: Roopa Prabhu @ 2018-07-20 20:21 UTC (permalink / raw)
To: davem; +Cc: netdev
In-Reply-To: <1532118064-45513-1-git-send-email-roopa@cumulusnetworks.com>
From: Roopa Prabhu <roopa@cumulusnetworks.com>
- Add new vxlan_fdb_alloc helper
- rename existing vxlan_fdb_create into vxlan_fdb_update:
because it really creates or updates an existing
fdb entry
- move new fdb creation into a separate vxlan_fdb_create
Main motivation for this change is to introduce the ability
to decouple vxlan fdb creation and notify, used in a later patch.
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
drivers/net/vxlan.c | 91 ++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 62 insertions(+), 29 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index f6bb1d5..c8d5bff 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -636,9 +636,62 @@ static int vxlan_gro_complete(struct sock *sk, struct sk_buff *skb, int nhoff)
return eth_gro_complete(skb, nhoff + sizeof(struct vxlanhdr));
}
-/* Add new entry to forwarding table -- assumes lock held */
+static struct vxlan_fdb *vxlan_fdb_alloc(struct vxlan_dev *vxlan,
+ const u8 *mac, __u16 state,
+ __be32 src_vni, __u8 ndm_flags)
+{
+ struct vxlan_fdb *f;
+
+ f = kmalloc(sizeof(*f), GFP_ATOMIC);
+ if (!f)
+ return NULL;
+ f->state = state;
+ f->flags = ndm_flags;
+ f->updated = f->used = jiffies;
+ f->vni = src_vni;
+ INIT_LIST_HEAD(&f->remotes);
+ memcpy(f->eth_addr, mac, ETH_ALEN);
+
+ return f;
+}
+
static int vxlan_fdb_create(struct vxlan_dev *vxlan,
const u8 *mac, union vxlan_addr *ip,
+ __u16 state, __be16 port, __be32 src_vni,
+ __be32 vni, __u32 ifindex, __u8 ndm_flags,
+ struct vxlan_fdb **fdb)
+{
+ struct vxlan_rdst *rd = NULL;
+ struct vxlan_fdb *f;
+ int rc;
+
+ if (vxlan->cfg.addrmax &&
+ vxlan->addrcnt >= vxlan->cfg.addrmax)
+ return -ENOSPC;
+
+ netdev_dbg(vxlan->dev, "add %pM -> %pIS\n", mac, ip);
+ f = vxlan_fdb_alloc(vxlan, mac, state, src_vni, ndm_flags);
+ if (!f)
+ return -ENOMEM;
+
+ rc = vxlan_fdb_append(f, ip, port, vni, ifindex, &rd);
+ if (rc < 0) {
+ kfree(f);
+ return rc;
+ }
+
+ ++vxlan->addrcnt;
+ hlist_add_head_rcu(&f->hlist,
+ vxlan_fdb_head(vxlan, mac, src_vni));
+
+ *fdb = f;
+
+ return 0;
+}
+
+/* Add new entry to forwarding table -- assumes lock held */
+static int vxlan_fdb_update(struct vxlan_dev *vxlan,
+ const u8 *mac, union vxlan_addr *ip,
__u16 state, __u16 flags,
__be16 port, __be32 src_vni, __be32 vni,
__u32 ifindex, __u8 ndm_flags)
@@ -687,37 +740,17 @@ static int vxlan_fdb_create(struct vxlan_dev *vxlan,
if (!(flags & NLM_F_CREATE))
return -ENOENT;
- if (vxlan->cfg.addrmax &&
- vxlan->addrcnt >= vxlan->cfg.addrmax)
- return -ENOSPC;
-
/* Disallow replace to add a multicast entry */
if ((flags & NLM_F_REPLACE) &&
(is_multicast_ether_addr(mac) || is_zero_ether_addr(mac)))
return -EOPNOTSUPP;
netdev_dbg(vxlan->dev, "add %pM -> %pIS\n", mac, ip);
- f = kmalloc(sizeof(*f), GFP_ATOMIC);
- if (!f)
- return -ENOMEM;
-
- notify = 1;
- f->state = state;
- f->flags = ndm_flags;
- f->updated = f->used = jiffies;
- f->vni = src_vni;
- INIT_LIST_HEAD(&f->remotes);
- memcpy(f->eth_addr, mac, ETH_ALEN);
-
- rc = vxlan_fdb_append(f, ip, port, vni, ifindex, &rd);
- if (rc < 0) {
- kfree(f);
+ rc = vxlan_fdb_create(vxlan, mac, ip, state, port, src_vni,
+ vni, ifindex, ndm_flags, &f);
+ if (rc < 0)
return rc;
- }
-
- ++vxlan->addrcnt;
- hlist_add_head_rcu(&f->hlist,
- vxlan_fdb_head(vxlan, mac, src_vni));
+ notify = 1;
}
if (notify) {
@@ -863,7 +896,7 @@ static int vxlan_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
return -EAFNOSUPPORT;
spin_lock_bh(&vxlan->hash_lock);
- err = vxlan_fdb_create(vxlan, addr, &ip, ndm->ndm_state, flags,
+ err = vxlan_fdb_update(vxlan, addr, &ip, ndm->ndm_state, flags,
port, src_vni, vni, ifindex, ndm->ndm_flags);
spin_unlock_bh(&vxlan->hash_lock);
@@ -1006,7 +1039,7 @@ static bool vxlan_snoop(struct net_device *dev,
/* close off race between vxlan_flush and incoming packets */
if (netif_running(dev))
- vxlan_fdb_create(vxlan, src_mac, src_ip,
+ vxlan_fdb_update(vxlan, src_mac, src_ip,
NUD_REACHABLE,
NLM_F_EXCL|NLM_F_CREATE,
vxlan->cfg.dst_port,
@@ -3170,7 +3203,7 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
/* create an fdb entry for a valid default destination */
if (!vxlan_addr_any(&vxlan->default_dst.remote_ip)) {
- err = vxlan_fdb_create(vxlan, all_zeros_mac,
+ err = vxlan_fdb_update(vxlan, all_zeros_mac,
&vxlan->default_dst.remote_ip,
NUD_REACHABLE | NUD_PERMANENT,
NLM_F_EXCL | NLM_F_CREATE,
@@ -3450,7 +3483,7 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
old_dst.remote_ifindex, 0);
if (!vxlan_addr_any(&dst->remote_ip)) {
- err = vxlan_fdb_create(vxlan, all_zeros_mac,
+ err = vxlan_fdb_update(vxlan, all_zeros_mac,
&dst->remote_ip,
NUD_REACHABLE | NUD_PERMANENT,
NLM_F_CREATE | NLM_F_APPEND,
--
2.1.4
^ permalink raw reply related
* [PATCH net 3/4] vxlan: make netlink notify in vxlan_fdb_destroy optional
From: Roopa Prabhu @ 2018-07-20 20:21 UTC (permalink / raw)
To: davem; +Cc: netdev
In-Reply-To: <1532118064-45513-1-git-send-email-roopa@cumulusnetworks.com>
From: Roopa Prabhu <roopa@cumulusnetworks.com>
Add a new option do_notify to vxlan_fdb_destroy to make
sending netlink notify optional. Used by a later patch.
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
drivers/net/vxlan.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index c8d5bff..a7e9a4d 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -774,13 +774,15 @@ static void vxlan_fdb_free(struct rcu_head *head)
kfree(f);
}
-static void vxlan_fdb_destroy(struct vxlan_dev *vxlan, struct vxlan_fdb *f)
+static void vxlan_fdb_destroy(struct vxlan_dev *vxlan, struct vxlan_fdb *f,
+ bool do_notify)
{
netdev_dbg(vxlan->dev,
"delete %pM\n", f->eth_addr);
--vxlan->addrcnt;
- vxlan_fdb_notify(vxlan, f, first_remote_rtnl(f), RTM_DELNEIGH);
+ if (do_notify)
+ vxlan_fdb_notify(vxlan, f, first_remote_rtnl(f), RTM_DELNEIGH);
hlist_del_rcu(&f->hlist);
call_rcu(&f->rcu, vxlan_fdb_free);
@@ -930,7 +932,7 @@ static int __vxlan_fdb_delete(struct vxlan_dev *vxlan,
goto out;
}
- vxlan_fdb_destroy(vxlan, f);
+ vxlan_fdb_destroy(vxlan, f, true);
out:
return 0;
@@ -2397,7 +2399,7 @@ static void vxlan_cleanup(struct timer_list *t)
"garbage collect %pM\n",
f->eth_addr);
f->state = NUD_STALE;
- vxlan_fdb_destroy(vxlan, f);
+ vxlan_fdb_destroy(vxlan, f, true);
} else if (time_before(timeout, next_timer))
next_timer = timeout;
}
@@ -2448,7 +2450,7 @@ static void vxlan_fdb_delete_default(struct vxlan_dev *vxlan, __be32 vni)
spin_lock_bh(&vxlan->hash_lock);
f = __vxlan_find_mac(vxlan, all_zeros_mac, vni);
if (f)
- vxlan_fdb_destroy(vxlan, f);
+ vxlan_fdb_destroy(vxlan, f, true);
spin_unlock_bh(&vxlan->hash_lock);
}
@@ -2502,7 +2504,7 @@ static void vxlan_flush(struct vxlan_dev *vxlan, bool do_all)
continue;
/* the all_zeros_mac entry is deleted at vxlan_uninit */
if (!is_zero_ether_addr(f->eth_addr))
- vxlan_fdb_destroy(vxlan, f);
+ vxlan_fdb_destroy(vxlan, f, true);
}
}
spin_unlock_bh(&vxlan->hash_lock);
--
2.1.4
^ permalink raw reply related
* [PATCH net 4/4] vxlan: fix default fdb entry netlink notify ordering during netdev create
From: Roopa Prabhu @ 2018-07-20 20:21 UTC (permalink / raw)
To: davem; +Cc: netdev
In-Reply-To: <1532118064-45513-1-git-send-email-roopa@cumulusnetworks.com>
From: Roopa Prabhu <roopa@cumulusnetworks.com>
Problem:
In vxlan_newlink, a default fdb entry is added before register_netdev.
The default fdb creation function also notifies user-space of the
fdb entry on the vxlan device which user-space does not know about yet.
(RTM_NEWNEIGH goes before RTM_NEWLINK for the same ifindex).
This patch fixes the user-space netlink notification ordering issue
with the following changes:
- decouple fdb notify from fdb create.
- Move fdb notify after register_netdev.
- Call rtnl_configure_link in vxlan newlink handler to notify
userspace about the newlink before fdb notify and
hence avoiding the user-space race.
Fixes: afbd8bae9c79 ("vxlan: add implicit fdb entry for default destination")
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
drivers/net/vxlan.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index a7e9a4d..e857cb3 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -3195,6 +3195,7 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
{
struct vxlan_net *vn = net_generic(net, vxlan_net_id);
struct vxlan_dev *vxlan = netdev_priv(dev);
+ struct vxlan_fdb *f = NULL;
int err;
err = vxlan_dev_configure(net, dev, conf, false, extack);
@@ -3205,27 +3206,38 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
/* create an fdb entry for a valid default destination */
if (!vxlan_addr_any(&vxlan->default_dst.remote_ip)) {
- err = vxlan_fdb_update(vxlan, all_zeros_mac,
+ err = vxlan_fdb_create(vxlan, all_zeros_mac,
&vxlan->default_dst.remote_ip,
NUD_REACHABLE | NUD_PERMANENT,
- NLM_F_EXCL | NLM_F_CREATE,
vxlan->cfg.dst_port,
vxlan->default_dst.remote_vni,
vxlan->default_dst.remote_vni,
vxlan->default_dst.remote_ifindex,
- NTF_SELF);
+ NTF_SELF, &f);
if (err)
return err;
}
err = register_netdevice(dev);
+ if (err)
+ goto errout;
+
+ err = rtnl_configure_link(dev, NULL);
if (err) {
- vxlan_fdb_delete_default(vxlan, vxlan->default_dst.remote_vni);
- return err;
+ unregister_netdevice(dev);
+ goto errout;
}
+ /* notify default fdb entry */
+ if (f)
+ vxlan_fdb_notify(vxlan, f, first_remote_rtnl(f), RTM_NEWNEIGH);
+
list_add(&vxlan->next, &vn->vxlan_list);
return 0;
+errout:
+ if (f)
+ vxlan_fdb_destroy(vxlan, f, false);
+ return err;
}
static int vxlan_nl2conf(struct nlattr *tb[], struct nlattr *data[],
@@ -3460,6 +3472,7 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
struct vxlan_rdst *dst = &vxlan->default_dst;
struct vxlan_rdst old_dst;
struct vxlan_config conf;
+ struct vxlan_fdb *f = NULL;
int err;
err = vxlan_nl2conf(tb, data,
@@ -3485,19 +3498,19 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
old_dst.remote_ifindex, 0);
if (!vxlan_addr_any(&dst->remote_ip)) {
- err = vxlan_fdb_update(vxlan, all_zeros_mac,
+ err = vxlan_fdb_create(vxlan, all_zeros_mac,
&dst->remote_ip,
NUD_REACHABLE | NUD_PERMANENT,
- NLM_F_CREATE | NLM_F_APPEND,
vxlan->cfg.dst_port,
dst->remote_vni,
dst->remote_vni,
dst->remote_ifindex,
- NTF_SELF);
+ NTF_SELF, &f);
if (err) {
spin_unlock_bh(&vxlan->hash_lock);
return err;
}
+ vxlan_fdb_notify(vxlan, f, first_remote_rtnl(f), RTM_NEWNEIGH);
}
spin_unlock_bh(&vxlan->hash_lock);
}
--
2.1.4
^ permalink raw reply related
* Re: [PATCH 2/4 v1] net: dsa: Add bindings for Realtek SMI DSAs
From: Linus Walleij @ 2018-07-20 20:26 UTC (permalink / raw)
To: Rob Herring
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, netdev,
OpenWrt Development List, LEDE Development List,
Antti Seppälä, Roman Yeryomin, Colin Leitner,
Gabor Juhos,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
In-Reply-To: <20180720161756.GA32587@rob-hp-laptop>
On Fri, Jul 20, 2018 at 6:17 PM Rob Herring <robh@kernel.org> wrote:
> On Tue, Jul 17, 2018 at 08:55:51AM +0200, Linus Walleij wrote:
> > It's just very different.
>
> Okay. Just wanted to make sure.
>
> Reviewed-by: Rob Herring <robh@kernel.org>
Thanks Rob! And thanks for asking the critical questions, more
often than not I am wrong (like that TPO screen with "custom"
wire protocol that turned out to be an SPI dialect), so it's good
to take this time to go back and check things over.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH net-next v3 0/8] Make /sys/class/net per net namespace objects belong to container
From: Tyler Hicks @ 2018-07-20 21:56 UTC (permalink / raw)
To: Greg Kroah-Hartman, Tejun Heo, David S. Miller, Stephen Hemminger
Cc: Dmitry Torokhov, Eric W. Biederman, linux-kernel, netdev, bridge,
Linux Containers
This is a revival of an older patch set from Dmitry Torokhov:
https://lore.kernel.org/lkml/1471386795-32918-1-git-send-email-dmitry.torokhov@gmail.com/
My submission of v2 is here:
https://lore.kernel.org/lkml/1531497949-1766-1-git-send-email-tyhicks@canonical.com/
Here's Dmitry's description:
There are objects in /sys hierarchy (/sys/class/net/) that logically
belong to a namespace/container. Unfortunately all sysfs objects start
their life belonging to global root, and while we could change
ownership manually, keeping tracks of all objects that come and go is
cumbersome. It would be better if kernel created them using correct
uid/gid from the beginning.
This series changes kernfs to allow creating object's with arbitrary
uid/gid, adds get_ownership() callback to ktype structure so subsystems
could supply their own logic (likely tied to namespace support) for
determining ownership of kobjects, and adjusts sysfs code to make use
of this information. Lastly net-sysfs is adjusted to make sure that
objects in net namespace are owned by the root user from the owning
user namespace.
Note that we do not adjust ownership of objects moved into a new
namespace (as when moving a network device into a container) as
userspace can easily do it.
I'm reviving this patch set because we would like this feature for
system containers. One specific use case that we have is that libvirt is
unable to configure its bridge device inside of a system container due
to the bridge files in /sys/class/net/ being owned by init root instead
of container root. The last two patches in this set are patches that
I've added to Dmitry's original set to allow such configuration of the
bridge device.
Eric had previously provided feedback that he didn't favor these changes
affecting all layers of the stack and that most of the changes could
remain local to drivers/base/core.c. That feedback is certainly sensible
but I wanted to send out v2 of the patch set without making that large
of a change since quite a bit of time has passed and the bridge changes
in the last patch of this set shows that not all of the changes will be
local to drivers/base/core.c. I'm happy to make the changes if the
original request still stands.
* Changes since v2:
- Added my Co-Developed-by and Signed-off-by tags to all of Dmitry's
patches that I've modified
- Patch 1 received build failure fixes in
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
- Patch 2 was updated to drop the declaration of sysfs_add_file() from
sysfs.h since the patch removed all other uses of the function
- Patch 5 is a new patch that prevents tx_maxrate from being written
to from inside of a container
+ Maybe I'm being too cautious here but the restriction can always
be loosened up later
- Patches 6 and 7 were updated to make net_ns_get_ownership() always
initialize uid and gid, even when the network namespace is NULL, so
that it isn't a dangerous function to reuse
+ Requested by Christian Brauner
- I've looked at all sysfs attributes affected by this patch set and
feel comfortable about the changes. There are quite a few affected
attributes that don't have any capable()/ns_capable() checks in
their store operations (per_bond_attrs, at91_sysfs_attrs,
sysfs_grcan_attrs, ican3_sysfs_attrs, cdc_ncm_sysfs_attrs,
qmi_wwan_sysfs_attrs) but I think this is acceptable. It means that
container root, rather than specifically CAP_NET_ADMIN inside of the
network namespace that the device belongs to, can write to those
device attributes. It's the same situation that those devices have
today in that init root is able to write to the attributes without
necessarily having CAP_NET_ADMIN. I think that this should probably
be fixed in order to be consistent with what netdev_store() does by
verifying CAP_NET_ADMIN in the network namespace but that it doesn't
need to happen in this patch set.
Thanks!
Tyler
^ permalink raw reply
* [PATCH net-next v3 1/8] kernfs: allow creating kernfs objects with arbitrary uid/gid
From: Tyler Hicks @ 2018-07-20 21:56 UTC (permalink / raw)
To: Greg Kroah-Hartman, Tejun Heo, David S. Miller, Stephen Hemminger
Cc: Dmitry Torokhov, Eric W. Biederman, linux-kernel, netdev, bridge,
Linux Containers
In-Reply-To: <1532123814-1109-1-git-send-email-tyhicks@canonical.com>
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
This change allows creating kernfs files and directories with arbitrary
uid/gid instead of always using GLOBAL_ROOT_UID/GID by extending
kernfs_create_dir_ns() and kernfs_create_file_ns() with uid/gid arguments.
The "simple" kernfs_create_file() and kernfs_create_dir() are left alone
and always create objects belonging to the global root.
When creating symlinks ownership (uid/gid) is taken from the target kernfs
object.
Co-Developed-by: Tyler Hicks <tyhicks@canonical.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c | 4 +++-
fs/kernfs/dir.c | 29 ++++++++++++++++++++++++++---
fs/kernfs/file.c | 8 ++++++--
fs/kernfs/inode.c | 2 +-
fs/kernfs/kernfs-internal.h | 2 ++
fs/kernfs/symlink.c | 11 ++++++++++-
fs/sysfs/dir.c | 4 +++-
fs/sysfs/file.c | 5 +++--
include/linux/kernfs.h | 28 +++++++++++++++++++---------
kernel/cgroup/cgroup.c | 4 +++-
10 files changed, 76 insertions(+), 21 deletions(-)
diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
index 749856a2e736..9af1a21265d3 100644
--- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
+++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
@@ -146,6 +146,7 @@ static int rdtgroup_add_file(struct kernfs_node *parent_kn, struct rftype *rft)
int ret;
kn = __kernfs_create_file(parent_kn, rft->name, rft->mode,
+ GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
0, rft->kf_ops, rft, NULL, NULL);
if (IS_ERR(kn))
return PTR_ERR(kn);
@@ -1503,7 +1504,8 @@ static int mon_addfile(struct kernfs_node *parent_kn, const char *name,
struct kernfs_node *kn;
int ret = 0;
- kn = __kernfs_create_file(parent_kn, name, 0444, 0,
+ kn = __kernfs_create_file(parent_kn, name, 0444,
+ GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, 0,
&kf_mondata_ops, priv, NULL, NULL);
if (IS_ERR(kn))
return PTR_ERR(kn);
diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index d66cc0777303..4ca0b5c18192 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -619,6 +619,7 @@ struct kernfs_node *kernfs_node_from_dentry(struct dentry *dentry)
static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
const char *name, umode_t mode,
+ kuid_t uid, kgid_t gid,
unsigned flags)
{
struct kernfs_node *kn;
@@ -661,8 +662,22 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
kn->mode = mode;
kn->flags = flags;
+ if (!uid_eq(uid, GLOBAL_ROOT_UID) || !gid_eq(gid, GLOBAL_ROOT_GID)) {
+ struct iattr iattr = {
+ .ia_valid = ATTR_UID | ATTR_GID,
+ .ia_uid = uid,
+ .ia_gid = gid,
+ };
+
+ ret = __kernfs_setattr(kn, &iattr);
+ if (ret < 0)
+ goto err_out3;
+ }
+
return kn;
+ err_out3:
+ idr_remove(&root->ino_idr, kn->id.ino);
err_out2:
kmem_cache_free(kernfs_node_cache, kn);
err_out1:
@@ -672,11 +687,13 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
struct kernfs_node *kernfs_new_node(struct kernfs_node *parent,
const char *name, umode_t mode,
+ kuid_t uid, kgid_t gid,
unsigned flags)
{
struct kernfs_node *kn;
- kn = __kernfs_new_node(kernfs_root(parent), name, mode, flags);
+ kn = __kernfs_new_node(kernfs_root(parent),
+ name, mode, uid, gid, flags);
if (kn) {
kernfs_get(parent);
kn->parent = parent;
@@ -946,6 +963,7 @@ struct kernfs_root *kernfs_create_root(struct kernfs_syscall_ops *scops,
root->next_generation = 1;
kn = __kernfs_new_node(root, "", S_IFDIR | S_IRUGO | S_IXUGO,
+ GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
KERNFS_DIR);
if (!kn) {
idr_destroy(&root->ino_idr);
@@ -984,6 +1002,8 @@ void kernfs_destroy_root(struct kernfs_root *root)
* @parent: parent in which to create a new directory
* @name: name of the new directory
* @mode: mode of the new directory
+ * @uid: uid of the new directory
+ * @gid: gid of the new directory
* @priv: opaque data associated with the new directory
* @ns: optional namespace tag of the directory
*
@@ -991,13 +1011,15 @@ void kernfs_destroy_root(struct kernfs_root *root)
*/
struct kernfs_node *kernfs_create_dir_ns(struct kernfs_node *parent,
const char *name, umode_t mode,
+ kuid_t uid, kgid_t gid,
void *priv, const void *ns)
{
struct kernfs_node *kn;
int rc;
/* allocate */
- kn = kernfs_new_node(parent, name, mode | S_IFDIR, KERNFS_DIR);
+ kn = kernfs_new_node(parent, name, mode | S_IFDIR,
+ uid, gid, KERNFS_DIR);
if (!kn)
return ERR_PTR(-ENOMEM);
@@ -1028,7 +1050,8 @@ struct kernfs_node *kernfs_create_empty_dir(struct kernfs_node *parent,
int rc;
/* allocate */
- kn = kernfs_new_node(parent, name, S_IRUGO|S_IXUGO|S_IFDIR, KERNFS_DIR);
+ kn = kernfs_new_node(parent, name, S_IRUGO|S_IXUGO|S_IFDIR,
+ GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, KERNFS_DIR);
if (!kn)
return ERR_PTR(-ENOMEM);
diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 2015d8c45e4a..dbf5bc250bfd 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -965,6 +965,8 @@ const struct file_operations kernfs_file_fops = {
* @parent: directory to create the file in
* @name: name of the file
* @mode: mode of the file
+ * @uid: uid of the file
+ * @gid: gid of the file
* @size: size of the file
* @ops: kernfs operations for the file
* @priv: private data for the file
@@ -975,7 +977,8 @@ const struct file_operations kernfs_file_fops = {
*/
struct kernfs_node *__kernfs_create_file(struct kernfs_node *parent,
const char *name,
- umode_t mode, loff_t size,
+ umode_t mode, kuid_t uid, kgid_t gid,
+ loff_t size,
const struct kernfs_ops *ops,
void *priv, const void *ns,
struct lock_class_key *key)
@@ -986,7 +989,8 @@ struct kernfs_node *__kernfs_create_file(struct kernfs_node *parent,
flags = KERNFS_FILE;
- kn = kernfs_new_node(parent, name, (mode & S_IALLUGO) | S_IFREG, flags);
+ kn = kernfs_new_node(parent, name, (mode & S_IALLUGO) | S_IFREG,
+ uid, gid, flags);
if (!kn)
return ERR_PTR(-ENOMEM);
diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index 3d73fe9d56e2..80cebcd94c90 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -63,7 +63,7 @@ static struct kernfs_iattrs *kernfs_iattrs(struct kernfs_node *kn)
return ret;
}
-static int __kernfs_setattr(struct kernfs_node *kn, const struct iattr *iattr)
+int __kernfs_setattr(struct kernfs_node *kn, const struct iattr *iattr)
{
struct kernfs_iattrs *attrs;
struct iattr *iattrs;
diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
index 0f260dcca177..3d83b114bb08 100644
--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h
@@ -90,6 +90,7 @@ int kernfs_iop_setattr(struct dentry *dentry, struct iattr *iattr);
int kernfs_iop_getattr(const struct path *path, struct kstat *stat,
u32 request_mask, unsigned int query_flags);
ssize_t kernfs_iop_listxattr(struct dentry *dentry, char *buf, size_t size);
+int __kernfs_setattr(struct kernfs_node *kn, const struct iattr *iattr);
/*
* dir.c
@@ -104,6 +105,7 @@ void kernfs_put_active(struct kernfs_node *kn);
int kernfs_add_one(struct kernfs_node *kn);
struct kernfs_node *kernfs_new_node(struct kernfs_node *parent,
const char *name, umode_t mode,
+ kuid_t uid, kgid_t gid,
unsigned flags);
struct kernfs_node *kernfs_find_and_get_node_by_ino(struct kernfs_root *root,
unsigned int ino);
diff --git a/fs/kernfs/symlink.c b/fs/kernfs/symlink.c
index 08ccabd7047f..5ffed48f3d0e 100644
--- a/fs/kernfs/symlink.c
+++ b/fs/kernfs/symlink.c
@@ -21,6 +21,7 @@
* @target: target node for the symlink to point to
*
* Returns the created node on success, ERR_PTR() value on error.
+ * Ownership of the link matches ownership of the target.
*/
struct kernfs_node *kernfs_create_link(struct kernfs_node *parent,
const char *name,
@@ -28,8 +29,16 @@ struct kernfs_node *kernfs_create_link(struct kernfs_node *parent,
{
struct kernfs_node *kn;
int error;
+ kuid_t uid = GLOBAL_ROOT_UID;
+ kgid_t gid = GLOBAL_ROOT_GID;
- kn = kernfs_new_node(parent, name, S_IFLNK|S_IRWXUGO, KERNFS_LINK);
+ if (target->iattr) {
+ uid = target->iattr->ia_iattr.ia_uid;
+ gid = target->iattr->ia_iattr.ia_gid;
+ }
+
+ kn = kernfs_new_node(parent, name, S_IFLNK|S_IRWXUGO, uid, gid,
+ KERNFS_LINK);
if (!kn)
return ERR_PTR(-ENOMEM);
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 58eba92a0e41..e39b884f0867 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -52,7 +52,9 @@ int sysfs_create_dir_ns(struct kobject *kobj, const void *ns)
return -ENOENT;
kn = kernfs_create_dir_ns(parent, kobject_name(kobj),
- S_IRWXU | S_IRUGO | S_IXUGO, kobj, ns);
+ S_IRWXU | S_IRUGO | S_IXUGO,
+ GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
+ kobj, ns);
if (IS_ERR(kn)) {
if (PTR_ERR(kn) == -EEXIST)
sysfs_warn_dup(parent, kobject_name(kobj));
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 5c13f29bfcdb..513fa691ecbd 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -302,8 +302,9 @@ int sysfs_add_file_mode_ns(struct kernfs_node *parent,
if (!attr->ignore_lockdep)
key = attr->key ?: (struct lock_class_key *)&attr->skey;
#endif
- kn = __kernfs_create_file(parent, attr->name, mode & 0777, size, ops,
- (void *)attr, ns, key);
+ kn = __kernfs_create_file(parent, attr->name,
+ mode & 0777, GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
+ size, ops, (void *)attr, ns, key);
if (IS_ERR(kn)) {
if (PTR_ERR(kn) == -EEXIST)
sysfs_warn_dup(parent, attr->name);
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index ab25c8b6d9e3..814643f7ee52 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -15,6 +15,7 @@
#include <linux/lockdep.h>
#include <linux/rbtree.h>
#include <linux/atomic.h>
+#include <linux/uidgid.h>
#include <linux/wait.h>
struct file;
@@ -325,12 +326,14 @@ void kernfs_destroy_root(struct kernfs_root *root);
struct kernfs_node *kernfs_create_dir_ns(struct kernfs_node *parent,
const char *name, umode_t mode,
+ kuid_t uid, kgid_t gid,
void *priv, const void *ns);
struct kernfs_node *kernfs_create_empty_dir(struct kernfs_node *parent,
const char *name);
struct kernfs_node *__kernfs_create_file(struct kernfs_node *parent,
- const char *name,
- umode_t mode, loff_t size,
+ const char *name, umode_t mode,
+ kuid_t uid, kgid_t gid,
+ loff_t size,
const struct kernfs_ops *ops,
void *priv, const void *ns,
struct lock_class_key *key);
@@ -415,12 +418,14 @@ static inline void kernfs_destroy_root(struct kernfs_root *root) { }
static inline struct kernfs_node *
kernfs_create_dir_ns(struct kernfs_node *parent, const char *name,
- umode_t mode, void *priv, const void *ns)
+ umode_t mode, kuid_t uid, kgid_t gid,
+ void *priv, const void *ns)
{ return ERR_PTR(-ENOSYS); }
static inline struct kernfs_node *
__kernfs_create_file(struct kernfs_node *parent, const char *name,
- umode_t mode, loff_t size, const struct kernfs_ops *ops,
+ umode_t mode, kuid_t uid, kgid_t gid,
+ loff_t size, const struct kernfs_ops *ops,
void *priv, const void *ns, struct lock_class_key *key)
{ return ERR_PTR(-ENOSYS); }
@@ -498,12 +503,15 @@ static inline struct kernfs_node *
kernfs_create_dir(struct kernfs_node *parent, const char *name, umode_t mode,
void *priv)
{
- return kernfs_create_dir_ns(parent, name, mode, priv, NULL);
+ return kernfs_create_dir_ns(parent, name, mode,
+ GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
+ priv, NULL);
}
static inline struct kernfs_node *
kernfs_create_file_ns(struct kernfs_node *parent, const char *name,
- umode_t mode, loff_t size, const struct kernfs_ops *ops,
+ umode_t mode, kuid_t uid, kgid_t gid,
+ loff_t size, const struct kernfs_ops *ops,
void *priv, const void *ns)
{
struct lock_class_key *key = NULL;
@@ -511,15 +519,17 @@ kernfs_create_file_ns(struct kernfs_node *parent, const char *name,
#ifdef CONFIG_DEBUG_LOCK_ALLOC
key = (struct lock_class_key *)&ops->lockdep_key;
#endif
- return __kernfs_create_file(parent, name, mode, size, ops, priv, ns,
- key);
+ return __kernfs_create_file(parent, name, mode, uid, gid,
+ size, ops, priv, ns, key);
}
static inline struct kernfs_node *
kernfs_create_file(struct kernfs_node *parent, const char *name, umode_t mode,
loff_t size, const struct kernfs_ops *ops, void *priv)
{
- return kernfs_create_file_ns(parent, name, mode, size, ops, priv, NULL);
+ return kernfs_create_file_ns(parent, name, mode,
+ GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
+ size, ops, priv, NULL);
}
static inline int kernfs_remove_by_name(struct kernfs_node *parent,
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 077370bf8964..35cf3d71f8aa 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -3557,7 +3557,9 @@ static int cgroup_add_file(struct cgroup_subsys_state *css, struct cgroup *cgrp,
key = &cft->lockdep_key;
#endif
kn = __kernfs_create_file(cgrp->kn, cgroup_file_name(cgrp, cft, name),
- cgroup_file_mode(cft), 0, cft->kf_ops, cft,
+ cgroup_file_mode(cft),
+ GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
+ 0, cft->kf_ops, cft,
NULL, key);
if (IS_ERR(kn))
return PTR_ERR(kn);
--
2.7.4
^ permalink raw reply related
* [PATCH net-next v3 2/8] sysfs, kobject: allow creating kobject belonging to arbitrary users
From: Tyler Hicks @ 2018-07-20 21:56 UTC (permalink / raw)
To: Greg Kroah-Hartman, Tejun Heo, David S. Miller, Stephen Hemminger
Cc: Dmitry Torokhov, Eric W. Biederman, linux-kernel, netdev, bridge,
Linux Containers
In-Reply-To: <1532123814-1109-1-git-send-email-tyhicks@canonical.com>
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Normally kobjects and their sysfs representation belong to global root,
however it is not necessarily the case for objects in separate namespaces.
For example, objects in separate network namespace logically belong to the
container's root and not global root.
This change lays groundwork for allowing network namespace objects
ownership to be transferred to container's root user by defining
get_ownership() callback in ktype structure and using it in sysfs code to
retrieve desired uid/gid when creating sysfs objects for given kobject.
Co-Developed-by: Tyler Hicks <tyhicks@canonical.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
fs/sysfs/dir.c | 7 +++++--
fs/sysfs/file.c | 32 ++++++++++++++++++++------------
fs/sysfs/group.c | 23 +++++++++++++++++------
fs/sysfs/sysfs.h | 5 ++---
include/linux/kobject.h | 4 ++++
lib/kobject.c | 19 +++++++++++++++++++
6 files changed, 67 insertions(+), 23 deletions(-)
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index e39b884f0867..feeae8081c22 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -40,6 +40,8 @@ void sysfs_warn_dup(struct kernfs_node *parent, const char *name)
int sysfs_create_dir_ns(struct kobject *kobj, const void *ns)
{
struct kernfs_node *parent, *kn;
+ kuid_t uid;
+ kgid_t gid;
BUG_ON(!kobj);
@@ -51,9 +53,10 @@ int sysfs_create_dir_ns(struct kobject *kobj, const void *ns)
if (!parent)
return -ENOENT;
+ kobject_get_ownership(kobj, &uid, &gid);
+
kn = kernfs_create_dir_ns(parent, kobject_name(kobj),
- S_IRWXU | S_IRUGO | S_IXUGO,
- GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
+ S_IRWXU | S_IRUGO | S_IXUGO, uid, gid,
kobj, ns);
if (IS_ERR(kn)) {
if (PTR_ERR(kn) == -EEXIST)
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 513fa691ecbd..fa46216523cf 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -245,7 +245,7 @@ static const struct kernfs_ops sysfs_bin_kfops_mmap = {
int sysfs_add_file_mode_ns(struct kernfs_node *parent,
const struct attribute *attr, bool is_bin,
- umode_t mode, const void *ns)
+ umode_t mode, kuid_t uid, kgid_t gid, const void *ns)
{
struct lock_class_key *key = NULL;
const struct kernfs_ops *ops;
@@ -302,8 +302,8 @@ int sysfs_add_file_mode_ns(struct kernfs_node *parent,
if (!attr->ignore_lockdep)
key = attr->key ?: (struct lock_class_key *)&attr->skey;
#endif
- kn = __kernfs_create_file(parent, attr->name,
- mode & 0777, GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
+
+ kn = __kernfs_create_file(parent, attr->name, mode & 0777, uid, gid,
size, ops, (void *)attr, ns, key);
if (IS_ERR(kn)) {
if (PTR_ERR(kn) == -EEXIST)
@@ -313,12 +313,6 @@ int sysfs_add_file_mode_ns(struct kernfs_node *parent,
return 0;
}
-int sysfs_add_file(struct kernfs_node *parent, const struct attribute *attr,
- bool is_bin)
-{
- return sysfs_add_file_mode_ns(parent, attr, is_bin, attr->mode, NULL);
-}
-
/**
* sysfs_create_file_ns - create an attribute file for an object with custom ns
* @kobj: object we're creating for
@@ -328,9 +322,14 @@ int sysfs_add_file(struct kernfs_node *parent, const struct attribute *attr,
int sysfs_create_file_ns(struct kobject *kobj, const struct attribute *attr,
const void *ns)
{
+ kuid_t uid;
+ kgid_t gid;
+
BUG_ON(!kobj || !kobj->sd || !attr);
- return sysfs_add_file_mode_ns(kobj->sd, attr, false, attr->mode, ns);
+ kobject_get_ownership(kobj, &uid, &gid);
+ return sysfs_add_file_mode_ns(kobj->sd, attr, false, attr->mode,
+ uid, gid, ns);
}
EXPORT_SYMBOL_GPL(sysfs_create_file_ns);
@@ -359,6 +358,8 @@ int sysfs_add_file_to_group(struct kobject *kobj,
const struct attribute *attr, const char *group)
{
struct kernfs_node *parent;
+ kuid_t uid;
+ kgid_t gid;
int error;
if (group) {
@@ -371,7 +372,9 @@ int sysfs_add_file_to_group(struct kobject *kobj,
if (!parent)
return -ENOENT;
- error = sysfs_add_file(parent, attr, false);
+ kobject_get_ownership(kobj, &uid, &gid);
+ error = sysfs_add_file_mode_ns(kobj->sd, attr, false,
+ attr->mode, uid, gid, NULL);
kernfs_put(parent);
return error;
@@ -487,9 +490,14 @@ EXPORT_SYMBOL_GPL(sysfs_remove_file_from_group);
int sysfs_create_bin_file(struct kobject *kobj,
const struct bin_attribute *attr)
{
+ kuid_t uid;
+ kgid_t gid;
+
BUG_ON(!kobj || !kobj->sd || !attr);
- return sysfs_add_file(kobj->sd, &attr->attr, true);
+ kobject_get_ownership(kobj, &uid, &gid);
+ return sysfs_add_file_mode_ns(kobj->sd, &attr->attr, true,
+ attr->attr.mode, uid, gid, NULL);
}
EXPORT_SYMBOL_GPL(sysfs_create_bin_file);
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 4802ec0e1e3a..c7a716c4acc9 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -31,6 +31,7 @@ static void remove_files(struct kernfs_node *parent,
}
static int create_files(struct kernfs_node *parent, struct kobject *kobj,
+ kuid_t uid, kgid_t gid,
const struct attribute_group *grp, int update)
{
struct attribute *const *attr;
@@ -60,7 +61,7 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
mode &= SYSFS_PREALLOC | 0664;
error = sysfs_add_file_mode_ns(parent, *attr, false,
- mode, NULL);
+ mode, uid, gid, NULL);
if (unlikely(error))
break;
}
@@ -90,7 +91,8 @@ static int create_files(struct kernfs_node *parent, struct kobject *kobj,
mode &= SYSFS_PREALLOC | 0664;
error = sysfs_add_file_mode_ns(parent,
&(*bin_attr)->attr, true,
- mode, NULL);
+ mode,
+ uid, gid, NULL);
if (error)
break;
}
@@ -106,6 +108,8 @@ static int internal_create_group(struct kobject *kobj, int update,
const struct attribute_group *grp)
{
struct kernfs_node *kn;
+ kuid_t uid;
+ kgid_t gid;
int error;
BUG_ON(!kobj || (!update && !kobj->sd));
@@ -118,9 +122,11 @@ static int internal_create_group(struct kobject *kobj, int update,
kobj->name, grp->name ?: "");
return -EINVAL;
}
+ kobject_get_ownership(kobj, &uid, &gid);
if (grp->name) {
- kn = kernfs_create_dir(kobj->sd, grp->name,
- S_IRWXU | S_IRUGO | S_IXUGO, kobj);
+ kn = kernfs_create_dir_ns(kobj->sd, grp->name,
+ S_IRWXU | S_IRUGO | S_IXUGO,
+ uid, gid, kobj, NULL);
if (IS_ERR(kn)) {
if (PTR_ERR(kn) == -EEXIST)
sysfs_warn_dup(kobj->sd, grp->name);
@@ -129,7 +135,7 @@ static int internal_create_group(struct kobject *kobj, int update,
} else
kn = kobj->sd;
kernfs_get(kn);
- error = create_files(kn, kobj, grp, update);
+ error = create_files(kn, kobj, uid, gid, grp, update);
if (error) {
if (grp->name)
kernfs_remove(kn);
@@ -281,6 +287,8 @@ int sysfs_merge_group(struct kobject *kobj,
const struct attribute_group *grp)
{
struct kernfs_node *parent;
+ kuid_t uid;
+ kgid_t gid;
int error = 0;
struct attribute *const *attr;
int i;
@@ -289,8 +297,11 @@ int sysfs_merge_group(struct kobject *kobj,
if (!parent)
return -ENOENT;
+ kobject_get_ownership(kobj, &uid, &gid);
+
for ((i = 0, attr = grp->attrs); *attr && !error; (++i, ++attr))
- error = sysfs_add_file(parent, *attr, false);
+ error = sysfs_add_file_mode_ns(parent, *attr, false,
+ (*attr)->mode, uid, gid, NULL);
if (error) {
while (--i >= 0)
kernfs_remove_by_name(parent, (*--attr)->name);
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index d098e015fcc9..0050cc0c0236 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -27,11 +27,10 @@ void sysfs_warn_dup(struct kernfs_node *parent, const char *name);
/*
* file.c
*/
-int sysfs_add_file(struct kernfs_node *parent,
- const struct attribute *attr, bool is_bin);
int sysfs_add_file_mode_ns(struct kernfs_node *parent,
const struct attribute *attr, bool is_bin,
- umode_t amode, const void *ns);
+ umode_t amode, kuid_t uid, kgid_t gid,
+ const void *ns);
/*
* symlink.c
diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index 7f6f93c3df9c..b49ff230beba 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -26,6 +26,7 @@
#include <linux/wait.h>
#include <linux/atomic.h>
#include <linux/workqueue.h>
+#include <linux/uidgid.h>
#define UEVENT_HELPER_PATH_LEN 256
#define UEVENT_NUM_ENVP 32 /* number of env pointers */
@@ -114,6 +115,8 @@ extern struct kobject * __must_check kobject_get_unless_zero(
extern void kobject_put(struct kobject *kobj);
extern const void *kobject_namespace(struct kobject *kobj);
+extern void kobject_get_ownership(struct kobject *kobj,
+ kuid_t *uid, kgid_t *gid);
extern char *kobject_get_path(struct kobject *kobj, gfp_t flag);
struct kobj_type {
@@ -122,6 +125,7 @@ struct kobj_type {
struct attribute **default_attrs;
const struct kobj_ns_type_operations *(*child_ns_type)(struct kobject *kobj);
const void *(*namespace)(struct kobject *kobj);
+ void (*get_ownership)(struct kobject *kobj, kuid_t *uid, kgid_t *gid);
};
struct kobj_uevent_env {
diff --git a/lib/kobject.c b/lib/kobject.c
index 18989b5b3b56..f2dc1f756007 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -35,6 +35,25 @@ const void *kobject_namespace(struct kobject *kobj)
return kobj->ktype->namespace(kobj);
}
+/**
+ * kobject_get_ownership - get sysfs ownership data for @kobj
+ * @kobj: kobject in question
+ * @uid: kernel user ID for sysfs objects
+ * @gid: kernel group ID for sysfs objects
+ *
+ * Returns initial uid/gid pair that should be used when creating sysfs
+ * representation of given kobject. Normally used to adjust ownership of
+ * objects in a container.
+ */
+void kobject_get_ownership(struct kobject *kobj, kuid_t *uid, kgid_t *gid)
+{
+ *uid = GLOBAL_ROOT_UID;
+ *gid = GLOBAL_ROOT_GID;
+
+ if (kobj->ktype->get_ownership)
+ kobj->ktype->get_ownership(kobj, uid, gid);
+}
+
/*
* populate_dir - populate directory with attributes.
* @kobj: object we're working on.
--
2.7.4
^ permalink raw reply related
* [PATCH net-next v3 3/8] kobject: kset_create_and_add() - fetch ownership info from parent
From: Tyler Hicks @ 2018-07-20 21:56 UTC (permalink / raw)
To: Greg Kroah-Hartman, Tejun Heo, David S. Miller, Stephen Hemminger
Cc: Dmitry Torokhov, Eric W. Biederman, linux-kernel, netdev, bridge,
Linux Containers
In-Reply-To: <1532123814-1109-1-git-send-email-tyhicks@canonical.com>
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
This change implements get_ownership() for ksets created with
kset_create_and_add() call by fetching ownership data from parent kobject.
This is done mostly for benefit of "queues" attribute of net devices so
that corresponding directory belongs to container's root instead of global
root for network devices in a container.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Reviewed-by: Tyler Hicks <tyhicks@canonical.com>
---
lib/kobject.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/lib/kobject.c b/lib/kobject.c
index f2dc1f756007..389829d3a1d1 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -887,9 +887,16 @@ static void kset_release(struct kobject *kobj)
kfree(kset);
}
+void kset_get_ownership(struct kobject *kobj, kuid_t *uid, kgid_t *gid)
+{
+ if (kobj->parent)
+ kobject_get_ownership(kobj->parent, uid, gid);
+}
+
static struct kobj_type kset_ktype = {
.sysfs_ops = &kobj_sysfs_ops,
- .release = kset_release,
+ .release = kset_release,
+ .get_ownership = kset_get_ownership,
};
/**
--
2.7.4
^ permalink raw reply related
* [PATCH net-next v3 5/8] net-sysfs: require net admin in the init ns for setting tx_maxrate
From: Tyler Hicks @ 2018-07-20 21:56 UTC (permalink / raw)
To: Greg Kroah-Hartman, Tejun Heo, David S. Miller, Stephen Hemminger
Cc: Dmitry Torokhov, Eric W. Biederman, linux-kernel, netdev, bridge,
Linux Containers
In-Reply-To: <1532123814-1109-1-git-send-email-tyhicks@canonical.com>
An upcoming change will allow container root to open some /sys/class/net
files for writing. The tx_maxrate attribute can result in changes
to actual hardware devices so err on the side of caution by requiring
CAP_NET_ADMIN in the init namespace in the corresponding attribute store
operation.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
net/core/net-sysfs.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index ffa1d18f2c2c..405c41ecb20b 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -1087,6 +1087,9 @@ static ssize_t tx_maxrate_store(struct netdev_queue *queue,
int err, index = get_netdev_queue_index(queue);
u32 rate = 0;
+ if (!capable(CAP_NET_ADMIN))
+ return -EPERM;
+
err = kstrtou32(buf, 10, &rate);
if (err < 0)
return err;
--
2.7.4
^ permalink raw reply related
* [PATCH net-next v3 6/8] net-sysfs: make sure objects belong to container's owner
From: Tyler Hicks @ 2018-07-20 21:56 UTC (permalink / raw)
To: Greg Kroah-Hartman, Tejun Heo, David S. Miller, Stephen Hemminger
Cc: Dmitry Torokhov, Eric W. Biederman, linux-kernel, netdev, bridge,
Linux Containers
In-Reply-To: <1532123814-1109-1-git-send-email-tyhicks@canonical.com>
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
When creating various objects in /sys/class/net/... make sure that they
belong to container's owner instead of global root (if they belong to a
container/namespace).
Co-Developed-by: Tyler Hicks <tyhicks@canonical.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
net/core/net-sysfs.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 46 insertions(+), 1 deletion(-)
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 405c41ecb20b..ada065fc685e 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -656,6 +656,24 @@ static const struct attribute_group wireless_group = {
#define net_class_groups NULL
#endif /* CONFIG_SYSFS */
+static void net_ns_get_ownership(const struct net *net,
+ kuid_t *uid, kgid_t *gid)
+{
+ if (net) {
+ kuid_t ns_root_uid = make_kuid(net->user_ns, 0);
+ kgid_t ns_root_gid = make_kgid(net->user_ns, 0);
+
+ if (uid_valid(ns_root_uid))
+ *uid = ns_root_uid;
+
+ if (gid_valid(ns_root_gid))
+ *gid = ns_root_gid;
+ } else {
+ *uid = GLOBAL_ROOT_UID;
+ *gid = GLOBAL_ROOT_GID;
+ }
+}
+
#ifdef CONFIG_SYSFS
#define to_rx_queue_attr(_attr) \
container_of(_attr, struct rx_queue_attribute, attr)
@@ -905,11 +923,20 @@ static const void *rx_queue_namespace(struct kobject *kobj)
return ns;
}
+static void rx_queue_get_ownership(struct kobject *kobj,
+ kuid_t *uid, kgid_t *gid)
+{
+ const struct net *net = rx_queue_namespace(kobj);
+
+ net_ns_get_ownership(net, uid, gid);
+}
+
static struct kobj_type rx_queue_ktype __ro_after_init = {
.sysfs_ops = &rx_queue_sysfs_ops,
.release = rx_queue_release,
.default_attrs = rx_queue_default_attrs,
- .namespace = rx_queue_namespace
+ .namespace = rx_queue_namespace,
+ .get_ownership = rx_queue_get_ownership,
};
static int rx_queue_add_kobject(struct net_device *dev, int index)
@@ -1431,11 +1458,20 @@ static const void *netdev_queue_namespace(struct kobject *kobj)
return ns;
}
+static void netdev_queue_get_ownership(struct kobject *kobj,
+ kuid_t *uid, kgid_t *gid)
+{
+ const struct net *net = netdev_queue_namespace(kobj);
+
+ net_ns_get_ownership(net, uid, gid);
+}
+
static struct kobj_type netdev_queue_ktype __ro_after_init = {
.sysfs_ops = &netdev_queue_sysfs_ops,
.release = netdev_queue_release,
.default_attrs = netdev_queue_default_attrs,
.namespace = netdev_queue_namespace,
+ .get_ownership = netdev_queue_get_ownership,
};
static int netdev_queue_add_kobject(struct net_device *dev, int index)
@@ -1625,6 +1661,14 @@ static const void *net_namespace(struct device *d)
return dev_net(dev);
}
+static void net_get_ownership(struct device *d, kuid_t *uid, kgid_t *gid)
+{
+ struct net_device *dev = to_net_dev(d);
+ const struct net *net = dev_net(dev);
+
+ net_ns_get_ownership(net, uid, gid);
+}
+
static struct class net_class __ro_after_init = {
.name = "net",
.dev_release = netdev_release,
@@ -1632,6 +1676,7 @@ static struct class net_class __ro_after_init = {
.dev_uevent = netdev_uevent,
.ns_type = &net_ns_type_operations,
.namespace = net_namespace,
+ .get_ownership = net_get_ownership,
};
#ifdef CONFIG_OF_NET
--
2.7.4
^ permalink raw reply related
* [PATCH net-next v3 7/8] net: create reusable function for getting ownership info of sysfs inodes
From: Tyler Hicks @ 2018-07-20 21:56 UTC (permalink / raw)
To: Greg Kroah-Hartman, Tejun Heo, David S. Miller, Stephen Hemminger
Cc: Dmitry Torokhov, Eric W. Biederman, linux-kernel, netdev, bridge,
Linux Containers
In-Reply-To: <1532123814-1109-1-git-send-email-tyhicks@canonical.com>
Make net_ns_get_ownership() reusable by networking code outside of core.
This is useful, for example, to allow bridge related sysfs files to be
owned by container root.
Add a function comment since this is a potentially dangerous function to
use given the way that kobject_get_ownership() works by initializing uid
and gid before calling .get_ownership().
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
include/net/net_namespace.h | 10 ++++++++++
net/core/net-sysfs.c | 18 ------------------
net/core/net_namespace.c | 28 ++++++++++++++++++++++++++++
3 files changed, 38 insertions(+), 18 deletions(-)
diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index a71264d75d7f..9b5fdc50519a 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -10,6 +10,7 @@
#include <linux/workqueue.h>
#include <linux/list.h>
#include <linux/sysctl.h>
+#include <linux/uidgid.h>
#include <net/flow.h>
#include <net/netns/core.h>
@@ -170,6 +171,8 @@ extern struct net init_net;
struct net *copy_net_ns(unsigned long flags, struct user_namespace *user_ns,
struct net *old_net);
+void net_ns_get_ownership(const struct net *net, kuid_t *uid, kgid_t *gid);
+
void net_ns_barrier(void);
#else /* CONFIG_NET_NS */
#include <linux/sched.h>
@@ -182,6 +185,13 @@ static inline struct net *copy_net_ns(unsigned long flags,
return old_net;
}
+static inline void net_ns_get_ownership(const struct net *net,
+ kuid_t *uid, kgid_t *gid)
+{
+ *uid = GLOBAL_ROOT_UID;
+ *gid = GLOBAL_ROOT_GID;
+}
+
static inline void net_ns_barrier(void) {}
#endif /* CONFIG_NET_NS */
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index ada065fc685e..0a95bcf64cdc 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -656,24 +656,6 @@ static const struct attribute_group wireless_group = {
#define net_class_groups NULL
#endif /* CONFIG_SYSFS */
-static void net_ns_get_ownership(const struct net *net,
- kuid_t *uid, kgid_t *gid)
-{
- if (net) {
- kuid_t ns_root_uid = make_kuid(net->user_ns, 0);
- kgid_t ns_root_gid = make_kgid(net->user_ns, 0);
-
- if (uid_valid(ns_root_uid))
- *uid = ns_root_uid;
-
- if (gid_valid(ns_root_gid))
- *gid = ns_root_gid;
- } else {
- *uid = GLOBAL_ROOT_UID;
- *gid = GLOBAL_ROOT_GID;
- }
-}
-
#ifdef CONFIG_SYSFS
#define to_rx_queue_attr(_attr) \
container_of(_attr, struct rx_queue_attribute, attr)
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index a11e03f920d3..738871af5efa 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -17,6 +17,7 @@
#include <linux/user_namespace.h>
#include <linux/net_namespace.h>
#include <linux/sched/task.h>
+#include <linux/uidgid.h>
#include <net/sock.h>
#include <net/netlink.h>
@@ -448,6 +449,33 @@ struct net *copy_net_ns(unsigned long flags,
return net;
}
+/**
+ * net_ns_get_ownership - get sysfs ownership data for @net
+ * @net: network namespace in question (can be NULL)
+ * @uid: kernel user ID for sysfs objects
+ * @gid: kernel group ID for sysfs objects
+ *
+ * Returns the uid/gid pair of root in the user namespace associated with the
+ * given network namespace.
+ */
+void net_ns_get_ownership(const struct net *net, kuid_t *uid, kgid_t *gid)
+{
+ if (net) {
+ kuid_t ns_root_uid = make_kuid(net->user_ns, 0);
+ kgid_t ns_root_gid = make_kgid(net->user_ns, 0);
+
+ if (uid_valid(ns_root_uid))
+ *uid = ns_root_uid;
+
+ if (gid_valid(ns_root_gid))
+ *gid = ns_root_gid;
+ } else {
+ *uid = GLOBAL_ROOT_UID;
+ *gid = GLOBAL_ROOT_GID;
+ }
+}
+EXPORT_SYMBOL_GPL(net_ns_get_ownership);
+
static void unhash_nsid(struct net *net, struct net *last)
{
struct net *tmp;
--
2.7.4
^ permalink raw reply related
* [PATCH net-next v3 8/8] bridge: make sure objects belong to container's owner
From: Tyler Hicks @ 2018-07-20 21:56 UTC (permalink / raw)
To: Greg Kroah-Hartman, Tejun Heo, David S. Miller, Stephen Hemminger
Cc: Dmitry Torokhov, Eric W. Biederman, linux-kernel, netdev, bridge,
Linux Containers
In-Reply-To: <1532123814-1109-1-git-send-email-tyhicks@canonical.com>
When creating various bridge objects in /sys/class/net/... make sure
that they belong to the container's owner instead of global root (if
they belong to a container/namespace).
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
net/bridge/br_if.c | 9 +++++++++
net/bridge/br_private.h | 2 ++
net/bridge/br_sysfs_if.c | 5 ++---
3 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 05e42d86882d..e7c8d55212aa 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -26,6 +26,7 @@
#include <net/sock.h>
#include <linux/if_vlan.h>
#include <net/switchdev.h>
+#include <net/net_namespace.h>
#include "br_private.h"
@@ -204,11 +205,19 @@ static void release_nbp(struct kobject *kobj)
kfree(p);
}
+static void brport_get_ownership(struct kobject *kobj, kuid_t *uid, kgid_t *gid)
+{
+ struct net_bridge_port *p = kobj_to_brport(kobj);
+
+ net_ns_get_ownership(dev_net(p->dev), uid, gid);
+}
+
static struct kobj_type brport_ktype = {
#ifdef CONFIG_SYSFS
.sysfs_ops = &brport_sysfs_ops,
#endif
.release = release_nbp,
+ .get_ownership = brport_get_ownership,
};
static void destroy_nbp(struct net_bridge_port *p)
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 5216a524b537..cf0005d2a4d0 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -283,6 +283,8 @@ struct net_bridge_port {
u16 group_fwd_mask;
};
+#define kobj_to_brport(obj) container_of(obj, struct net_bridge_port, kobj)
+
#define br_auto_port(p) ((p)->flags & BR_AUTO_MASK)
#define br_promisc_port(p) ((p)->flags & BR_PROMISC)
diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index f99c5bf5c906..ab4c7f8adf68 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -249,13 +249,12 @@ static const struct brport_attribute *brport_attrs[] = {
};
#define to_brport_attr(_at) container_of(_at, struct brport_attribute, attr)
-#define to_brport(obj) container_of(obj, struct net_bridge_port, kobj)
static ssize_t brport_show(struct kobject *kobj,
struct attribute *attr, char *buf)
{
struct brport_attribute *brport_attr = to_brport_attr(attr);
- struct net_bridge_port *p = to_brport(kobj);
+ struct net_bridge_port *p = kobj_to_brport(kobj);
if (!brport_attr->show)
return -EINVAL;
@@ -268,7 +267,7 @@ static ssize_t brport_store(struct kobject *kobj,
const char *buf, size_t count)
{
struct brport_attribute *brport_attr = to_brport_attr(attr);
- struct net_bridge_port *p = to_brport(kobj);
+ struct net_bridge_port *p = kobj_to_brport(kobj);
ssize_t ret = -EINVAL;
char *endp;
unsigned long val;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next v2 6/7] net: Create reusable function for getting ownership info of sysfs inodes
From: Tyler Hicks @ 2018-07-20 21:58 UTC (permalink / raw)
To: Christian Brauner
Cc: Greg Kroah-Hartman, Tejun Heo, David S. Miller, Stephen Hemminger,
Dmitry Torokhov, netdev, Linux Containers, bridge, linux-kernel,
Eric W. Biederman
In-Reply-To: <20180719143616.GA29715@mailbox.org>
[-- Attachment #1.1: Type: text/plain, Size: 4382 bytes --]
On 07/19/2018 09:36 AM, Christian Brauner wrote:
> On Fri, Jul 13, 2018 at 04:05:48PM +0000, Tyler Hicks wrote:
>> Make net_ns_get_ownership() reusable by networking code outside of core.
>> This is useful, for example, to allow bridge related sysfs files to be
>> owned by container root.
>>
>> Add a function comment since this is a potentially dangerous function to
>> use given the way that kobject_get_ownership() works by initializing uid
>> and gid before calling .get_ownership().
>>
>> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
>> ---
>> include/net/net_namespace.h | 7 +++++++
>> net/core/net-sysfs.c | 15 ---------------
>> net/core/net_namespace.c | 25 +++++++++++++++++++++++++
>> 3 files changed, 32 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
>> index a71264d75d7f..a257710527ce 100644
>> --- a/include/net/net_namespace.h
>> +++ b/include/net/net_namespace.h
>> @@ -170,6 +170,8 @@ extern struct net init_net;
>> struct net *copy_net_ns(unsigned long flags, struct user_namespace *user_ns,
>> struct net *old_net);
>>
>> +void net_ns_get_ownership(const struct net *net, kuid_t *uid, kgid_t *gid);
>> +
>> void net_ns_barrier(void);
>> #else /* CONFIG_NET_NS */
>> #include <linux/sched.h>
>> @@ -182,6 +184,11 @@ static inline struct net *copy_net_ns(unsigned long flags,
>> return old_net;
>> }
>>
>> +static inline void net_ns_get_ownership(const struct net *net,
>> + kuid_t *uid, kgid_t *gid)
>> +{
>> +}
>> +
>> static inline void net_ns_barrier(void) {}
>> #endif /* CONFIG_NET_NS */
>>
>> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
>> index 41d84c40fe51..a3ad8108d296 100644
>> --- a/net/core/net-sysfs.c
>> +++ b/net/core/net-sysfs.c
>> @@ -656,21 +656,6 @@ static const struct attribute_group wireless_group = {
>> #define net_class_groups NULL
>> #endif /* CONFIG_SYSFS */
>>
>> -static void net_ns_get_ownership(const struct net *net,
>> - kuid_t *uid, kgid_t *gid)
>> -{
>> - if (net) {
>> - kuid_t ns_root_uid = make_kuid(net->user_ns, 0);
>> - kgid_t ns_root_gid = make_kgid(net->user_ns, 0);
>> -
>> - if (uid_valid(ns_root_uid))
>> - *uid = ns_root_uid;
>> -
>> - if (gid_valid(ns_root_gid))
>> - *gid = ns_root_gid;
>> - }
>> -}
>> -
>> #ifdef CONFIG_SYSFS
>> #define to_rx_queue_attr(_attr) \
>> container_of(_attr, struct rx_queue_attribute, attr)
>> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
>> index a11e03f920d3..5257875fa84d 100644
>> --- a/net/core/net_namespace.c
>> +++ b/net/core/net_namespace.c
>> @@ -448,6 +448,31 @@ struct net *copy_net_ns(unsigned long flags,
>> return net;
>> }
>>
>> +/**
>> + * net_ns_get_ownership - get sysfs ownership data for @net
>> + * @net: network namespace in question (can be NULL)
>> + * @uid: kernel user ID for sysfs objects (must be GLOBAL_ROOT_UID)
>> + * @gid: kernel group ID for sysfs objects (must be GLOBAL_ROOT_GID)
>> + *
>> + * Returns the uid/gid pair of root in the user namespace associated with the
>> + * given network namespace. The caller must initialize @uid and @gid to
>> + * GLOBAL_ROOT_UID/GLOBAL_ROOT_GID before calling this function.
>
> If they must be so initialized why not just enforce this directly in the
> function? This way callers can rely on always getting back the correct
> permissions and the comment can be removed as well.
I agree and made this change in v3 of the patch set that I just sent
out. Thanks for the suggestion!
Tyler
>
> Christian
>
>> + */
>> +void net_ns_get_ownership(const struct net *net, kuid_t *uid, kgid_t *gid)
>> +{
>> + if (net) {
>> + kuid_t ns_root_uid = make_kuid(net->user_ns, 0);
>> + kgid_t ns_root_gid = make_kgid(net->user_ns, 0);
>> +
>> + if (uid_valid(ns_root_uid))
>> + *uid = ns_root_uid;
>> +
>> + if (gid_valid(ns_root_gid))
>> + *gid = ns_root_gid;
>> + }
>> +}
>> +EXPORT_SYMBOL_GPL(net_ns_get_ownership);
>> +
>> static void unhash_nsid(struct net *net, struct net *last)
>> {
>> struct net *tmp;
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Containers mailing list
>> Containers@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/containers
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 1/2] net: bridge: add support for raw sysfs port options
From: Stephen Hemminger @ 2018-07-20 21:14 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: netdev, roopa, anuradhak, bridge, wkok, davem
In-Reply-To: <C6B4C1AB-7991-45B3-9649-CC4A36D878A8@cumulusnetworks.com>
On Fri, 20 Jul 2018 20:47:26 +0300
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> On July 20, 2018 8:26:36 PM GMT+03:00, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> >On July 20, 2018 8:20:44 PM GMT+03:00, Stephen Hemminger
> ><stephen@networkplumber.org> wrote:
> >>On Fri, 20 Jul 2018 20:14:43 +0300
> >>Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> >>
> >>> >Casting away the const on the buf variable is going to cause
> >>warnings
> >>> >and should not be necessary.
> >>> >
> >>>
> >>> It doesn't when it's casted like that, the new line is changed to
> >>null byte so we need to drop
> >>> the const.
> >>
> >>Then change store function to take a char *?
> >
> >Sure, will do.
>
> Acrually I can't change sysfs_ops store prototype.
> That is the reason the bond also casts the ptr.
>
>
>
Ok.
^ permalink raw reply
* Re: [PATCH net] net: phy: consider PHY_IGNORE_INTERRUPT in phy_start_aneg_priv
From: Heiner Kallweit @ 2018-07-20 21:22 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <013532d7-ed33-354b-81d9-f69ad534ebe3@gmail.com>
On 20.07.2018 11:36, Florian Fainelli wrote:
>
>
> On 07/18/2018 11:15 PM, Heiner Kallweit wrote:
>> The situation described in the comment can occur also with
>> PHY_IGNORE_INTERRUPT, therefore change the condition to include it.
>
> Yes indeed! You might want to locate the offending commit to provide a
> fixes tag so this could be backported to stable trees.
>
Fixes: f555f34fdc58 ("net: phy: fix auto-negotiation stall due to unavailable interrupt")
Is it sufficient this way or better re-submit the patch?
> Also, for net-next, we may want to introduce a helper which checks for
> phydev->irq != PHY_POLL that we can use consistently as a way to tell
> that the conditions applies to either PHY_IGNORE_INTERRUPT or
> phydev->irq is valid?
>
Yes, will put it on my agenda. Thanks for the feedback.
Heiner
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/net/phy/phy.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index d2baedc4..914fe8e6 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -519,7 +519,7 @@ static int phy_start_aneg_priv(struct phy_device *phydev, bool sync)
>> * negotiation may already be done and aneg interrupt may not be
>> * generated.
>> */
>> - if (phy_interrupt_is_valid(phydev) && (phydev->state == PHY_AN)) {
>> + if (phydev->irq != PHY_POLL && phydev->state == PHY_AN) {
>> err = phy_aneg_done(phydev);
>> if (err > 0) {
>> trigger = true;
>>
>
^ permalink raw reply
* Re: [RFC PATCH ghak90 (was ghak32) V3 01/10] audit: add container id
From: Paul Moore @ 2018-07-20 22:13 UTC (permalink / raw)
To: rgb
Cc: cgroups, containers, linux-api, linux-audit, linux-fsdevel,
linux-kernel, netdev, ebiederm, luto, jlayton, carlos, dhowells,
viro, simo, Eric Paris, serge
In-Reply-To: <0377c3ced6bdbc44fe17f9a5679cb6eda4304024.1528304203.git.rgb@redhat.com>
On Wed, Jun 6, 2018 at 1:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Implement the proc fs write to set the audit container identifier of a
> process, emitting an AUDIT_CONTAINER_ID record to document the event.
>
> This is a write from the container orchestrator task to a proc entry of
> the form /proc/PID/audit_containerid where PID is the process ID of the
> newly created task that is to become the first task in a container, or
> an additional task added to a container.
>
> The write expects up to a u64 value (unset: 18446744073709551615).
>
> The writer must have capability CAP_AUDIT_CONTROL.
>
> This will produce a record such as this:
> type=CONTAINER_ID msg=audit(2018-06-06 12:39:29.636:26949) : op=set opid=2209 old-contid=18446744073709551615 contid=123456 pid=628 auid=root uid=root tty=ttyS0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 comm=bash exe=/usr/bin/bash res=yes
>
> The "op" field indicates an initial set. The "pid" to "ses" fields are
> the orchestrator while the "opid" field is the object's PID, the process
> being "contained". Old and new audit container identifier values are
> given in the "contid" fields, while res indicates its success.
>
> It is not permitted to unset or re-set the audit container identifier.
> A child inherits its parent's audit container identifier, but then can
> be set only once after.
>
> See: https://github.com/linux-audit/audit-kernel/issues/90
> See: https://github.com/linux-audit/audit-userspace/issues/51
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> fs/proc/base.c | 37 ++++++++++++++++++++++++
> include/linux/audit.h | 25 ++++++++++++++++
> include/uapi/linux/audit.h | 2 ++
> kernel/auditsc.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 135 insertions(+)
...
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -606,6 +621,16 @@ static inline bool audit_loginuid_set(struct task_struct *tsk)
> return uid_valid(audit_get_loginuid(tsk));
> }
>
> +static inline bool cid_valid(u64 contid)
> +{
> + return contid != AUDIT_CID_UNSET;
> +}
> +
> +static inline bool audit_contid_set(struct task_struct *tsk)
> +{
> + return cid_valid(audit_get_contid(tsk));
> +}
For the sake of consistency I think we should rename cid_valid() to
audit_contid_valid().
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 59ef7a81..611e926 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -956,6 +956,8 @@ int audit_alloc(struct task_struct *tsk)
> return -ENOMEM;
> info->loginuid = audit_get_loginuid(current);
> info->sessionid = audit_get_sessionid(current);
> + info->contid = audit_get_contid(current);
> + info->inherited = true;
First see my others comments in this patch about inheritence, but if
we decide that flagging inherited values is important we should
probably rename the "inherited" field to indicate that it applies to
just the "contid" field.
> tsk->audit = info;
>
> if (likely(!audit_ever_enabled))
> @@ -985,6 +987,8 @@ int audit_alloc(struct task_struct *tsk)
> struct audit_task_info init_struct_audit = {
> .loginuid = INVALID_UID,
> .sessionid = AUDIT_SID_UNSET,
> + .contid = AUDIT_CID_UNSET,
> + .inherited = true,
> .ctx = NULL,
> };
>
> @@ -2112,6 +2116,73 @@ int audit_set_loginuid(kuid_t loginuid)
> }
>
> /**
> + * audit_set_contid - set current task's audit_context contid
> + * @contid: contid value
> + *
> + * Returns 0 on success, -EPERM on permission failure.
> + *
> + * Called (set) from fs/proc/base.c::proc_contid_write().
> + */
> +int audit_set_contid(struct task_struct *task, u64 contid)
> +{
> + u64 oldcontid;
> + int rc = 0;
> + struct audit_buffer *ab;
> + uid_t uid;
> + struct tty_struct *tty;
> + char comm[sizeof(current->comm)];
> +
> + /* Can't set if audit disabled */
> + if (!task->audit)
> + return -ENOPROTOOPT;
> + oldcontid = audit_get_contid(task);
> + /* Don't allow the audit containerid to be unset */
> + if (!cid_valid(contid))
> + rc = -EINVAL;
> + /* if we don't have caps, reject */
> + else if (!capable(CAP_AUDIT_CONTROL))
> + rc = -EPERM;
> + /* if task has children or is not single-threaded, deny */
> + else if (!list_empty(&task->children))
> + rc = -EBUSY;
Is this safe without holding tasklist_lock? I worry we might be
vulnerable to a race with fork().
> + else if (!(thread_group_leader(task) && thread_group_empty(task)))
> + rc = -EALREADY;
Similar concern here as well, although related to threads.
> + /* it is already set, and not inherited from the parent, reject */
> + else if (cid_valid(oldcontid) && !task->audit->inherited)
> + rc = -EEXIST;
Maybe I'm missing something, but why do we care about preventing
reassigning the audit container ID in this case? The task is single
threaded and has no descendants at this point so it should be safe,
yes? So long as the task changing the audit container ID has
capable(CAP_AUDIT_CONTOL) it shouldn't matter, right?
Related, I'm questioning if we would ever care if the audit container
ID was inherited or not?
> + if (!rc) {
> + task_lock(task);
> + task->audit->contid = contid;
> + task->audit->inherited = false;
> + task_unlock(task);
I suspect the task_lock() may not be what we want here, but if we are
using task_lock() to protect the audit fields two things come to mind:
1. We should update the header comments for task_lock() in task.h to
indicate that it also protects ->audit.
2. Where else do we need to worry about taking this lock? At the very
least we should take this lock near the top of this function before we
check task->audit and not drop it until after we have set it, or
failed the operation for one of the reasons above.
> + }
> +
> + if (!audit_enabled)
> + return rc;
> +
> + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_ID);
> + if (!ab)
> + return rc;
> +
> + uid = from_kuid(&init_user_ns, task_uid(current));
> + tty = audit_get_tty(current);
> + audit_log_format(ab, "op=set opid=%d old-contid=%llu contid=%llu pid=%d uid=%u auid=%u tty=%s ses=%u",
> + task_tgid_nr(task), oldcontid, contid,
> + task_tgid_nr(current), uid
> + from_kuid(&init_user_ns, audit_get_loginuid(current)),
> + tty ? tty_name(tty) : "(none)",
> + audit_get_sessionid(current));
> + audit_put_tty(tty);
> + audit_log_task_context(ab);
> + audit_log_format(ab, " comm=");
> + audit_log_untrustedstring(ab, get_task_comm(comm, current));
> + audit_log_d_path_exe(ab, current->mm);
> + audit_log_format(ab, " res=%d", !rc);
> + audit_log_end(ab);
> + return rc;
> +}
> +
> +/**
> * __audit_mq_open - record audit data for a POSIX MQ open
> * @oflag: open flag
> * @mode: mode bits
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak90 (was ghak32) V3 02/10] audit: log container info of syscalls
From: Paul Moore @ 2018-07-20 22:13 UTC (permalink / raw)
To: rgb
Cc: cgroups, containers, linux-api, linux-audit, linux-fsdevel,
linux-kernel, netdev, ebiederm, luto, jlayton, carlos, dhowells,
viro, simo, Eric Paris, serge
In-Reply-To: <b839dc7fb29aae226b2a4602e7ef3c2a406ee90a.1528304203.git.rgb@redhat.com>
On Wed, Jun 6, 2018 at 1:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Create a new audit record AUDIT_CONTAINER to document the audit
> container identifier of a process if it is present.
>
> Called from audit_log_exit(), syscalls are covered.
>
> A sample raw event:
> type=SYSCALL msg=audit(1519924845.499:257): arch=c000003e syscall=257 success=yes exit=3 a0=ffffff9c a1=56374e1cef30 a2=241 a3=1b6 items=2 ppid=606 pid=635 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=3 comm="bash" exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key="tmpcontainerid"
> type=CWD msg=audit(1519924845.499:257): cwd="/root"
> type=PATH msg=audit(1519924845.499:257): item=0 name="/tmp/" inode=13863 dev=00:27 mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype= PARENT cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> type=PATH msg=audit(1519924845.499:257): item=1 name="/tmp/tmpcontainerid" inode=17729 dev=00:27 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> type=PROCTITLE msg=audit(1519924845.499:257): proctitle=62617368002D6300736C65657020313B206563686F2074657374203E202F746D702F746D70636F6E7461696E65726964
> type=CONTAINER msg=audit(1519924845.499:257): op=task contid=123458
>
> See: https://github.com/linux-audit/audit-kernel/issues/90
> See: https://github.com/linux-audit/audit-userspace/issues/51
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 7 +++++++
> include/uapi/linux/audit.h | 1 +
> kernel/audit.c | 23 +++++++++++++++++++++++
> kernel/auditsc.c | 3 +++
> 4 files changed, 34 insertions(+)
...
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -115,6 +115,7 @@
> #define AUDIT_REPLACE 1329 /* Replace auditd if this packet unanswerd */
> #define AUDIT_KERN_MODULE 1330 /* Kernel Module events */
> #define AUDIT_FANOTIFY 1331 /* Fanotify access decision */
> +#define AUDIT_CONTAINER 1332 /* Container ID */
I'm not sure I'm completely sold on the AUDIT_CONTAINER_ID and
AUDIT_CONTAINER record type names. From what I can tell
AUDIT_CONTAINER_ID seems to be used for audit container ID management
operations, e.g. setting the ID, whereas the AUDIT_CONTAINER is used
to tag events with the corresponding audit container ID. Assuming
that is correct, it seems like AUDIT_CONTAINER might be better served
if it was named AUDIT_CONTAINER_ID and if we could change
AUDIT_CONTAINER_ID to AUDIT_CONTAINER_OP/MGMT/etc. Thoughts?
> #define AUDIT_AVC 1400 /* SE Linux avc denial or grant */
> #define AUDIT_SELINUX_ERR 1401 /* Internal SE Linux Errors */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index e7478cb..5e150c6 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2048,6 +2048,29 @@ void audit_log_session_info(struct audit_buffer *ab)
> audit_log_format(ab, " auid=%u ses=%u", auid, sessionid);
> }
>
> +/*
> + * audit_log_contid - report container info
> + * @tsk: task to be recorded
> + * @context: task or local context for record
> + * @op: contid string description
> + */
> +int audit_log_contid(struct task_struct *tsk,
> + struct audit_context *context, char *op)
> +{
> + struct audit_buffer *ab;
> +
> + if (!audit_contid_set(tsk))
> + return 0;
> + /* Generate AUDIT_CONTAINER record with container ID */
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> + if (!ab)
> + return -ENOMEM;
> + audit_log_format(ab, "op=%s contid=%llu",
> + op, audit_get_contid(tsk));
Can you explain your reason for including an "op" field in this record
type? I've been looking at the rest of the patches in this patchset
and it seems to be used more as an indicator of the record's
generating context rather than any sort of audit container ID
operation.
> + audit_log_end(ab);
> + return 0;
> +}
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak90 (was ghak32) V3 03/10] audit: add containerid support for ptrace and signals
From: Paul Moore @ 2018-07-20 22:13 UTC (permalink / raw)
To: rgb
Cc: cgroups, containers, linux-api, linux-audit, linux-fsdevel,
linux-kernel, netdev, ebiederm, luto, jlayton, carlos, dhowells,
viro, simo, Eric Paris, serge
In-Reply-To: <29359e0e6bc34c74b3a2c3ce0cdfda77f530cf18.1528304204.git.rgb@redhat.com>
On Wed, Jun 6, 2018 at 1:01 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Add audit container identifier support to ptrace and signals. In
> particular, the "op" field provides a way to label the auxiliary record
> to which it is associated.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 11 +++++------
> kernel/audit.c | 13 +++++++------
> kernel/audit.h | 2 ++
> kernel/auditsc.c | 21 ++++++++++++++++-----
> 4 files changed, 30 insertions(+), 17 deletions(-)
...
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 5e150c6..ba304a8 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -142,6 +142,7 @@ struct audit_net {
> kuid_t audit_sig_uid = INVALID_UID;
> pid_t audit_sig_pid = -1;
> u32 audit_sig_sid = 0;
> +u64 audit_sig_cid = AUDIT_CID_UNSET;
>
> /* Records can be lost in several ways:
> 0) [suppressed in audit_alloc]
> @@ -1437,6 +1438,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> memcpy(sig_data->ctx, ctx, len);
> security_release_secctx(ctx, len);
> }
> + sig_data->cid = audit_sig_cid;
> audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
> sig_data, sizeof(*sig_data) + len);
> kfree(sig_data);
> @@ -2050,23 +2052,22 @@ void audit_log_session_info(struct audit_buffer *ab)
>
> /*
> * audit_log_contid - report container info
> - * @tsk: task to be recorded
> * @context: task or local context for record
> * @op: contid string description
> + * @contid: container ID to report
> */
> -int audit_log_contid(struct task_struct *tsk,
> - struct audit_context *context, char *op)
> +int audit_log_contid(struct audit_context *context,
> + char *op, u64 contid)
> {
> struct audit_buffer *ab;
>
> - if (!audit_contid_set(tsk))
> + if (!cid_valid(contid))
> return 0;
> /* Generate AUDIT_CONTAINER record with container ID */
> ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> if (!ab)
> return -ENOMEM;
> - audit_log_format(ab, "op=%s contid=%llu",
> - op, audit_get_contid(tsk));
> + audit_log_format(ab, "op=%s contid=%llu", op, contid);
> audit_log_end(ab);
> return 0;
> }
You might as well just make these changes to audit_log_contid() in
patch 2 when you first define audit_log_contid().
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak90 (was ghak32) V3 04/10] audit: add support for non-syscall auxiliary records
From: Paul Moore @ 2018-07-20 22:14 UTC (permalink / raw)
To: rgb
Cc: cgroups, containers, linux-api, linux-audit, linux-fsdevel,
linux-kernel, netdev, ebiederm, luto, jlayton, carlos, dhowells,
viro, simo, Eric Paris, serge
In-Reply-To: <dcd3d6e27c4beb8aec11805faeb4de7013132bd7.1528304204.git.rgb@redhat.com>
On Wed, Jun 6, 2018 at 1:01 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Standalone audit records have the timestamp and serial number generated
> on the fly and as such are unique, making them standalone. This new
> function audit_alloc_local() generates a local audit context that will
> be used only for a standalone record and its auxiliary record(s). The
> context is discarded immediately after the local associated records are
> produced.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 8 ++++++++
> kernel/auditsc.c | 25 +++++++++++++++++++++++--
> 2 files changed, 31 insertions(+), 2 deletions(-)
...
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -916,8 +916,11 @@ static inline void audit_free_aux(struct audit_context *context)
> static inline struct audit_context *audit_alloc_context(enum audit_state state)
> {
> struct audit_context *context;
> + gfp_t gfpflags;
>
> - context = kzalloc(sizeof(*context), GFP_KERNEL);
> + /* We can be called in atomic context via audit_tg() */
> + gfpflags = (in_atomic() || irqs_disabled()) ? GFP_ATOMIC : GFP_KERNEL;
Instead of trying to guess the proper gfp flags, and likely getting it
wrong at some point (the in_atomic() comment in preempt.h don't give
me the warm fuzzies), why not pass the gfp flags as an argument?
Right now it looks like we would only have two callers: audit_alloc()
and audit_audit_local(). The audit_alloc() invocation would simply
pass GFP_KERNEL and we could allow the audit_alloc_local() callers to
specify the gfp flags when calling audit_alloc_local() (although I
suspect that will always be GFP_ATOMIC since we should only be calling
audit_alloc_local() from interrupt-like context, in all other cases we
should use the audit_context from the current task_struct.
> + context = kzalloc(sizeof(*context), gfpflags);
> if (!context)
> return NULL;
> context->state = state;
> @@ -993,8 +996,26 @@ struct audit_task_info init_struct_audit = {
> .ctx = NULL,
> };
>
> -static inline void audit_free_context(struct audit_context *context)
> +struct audit_context *audit_alloc_local(void)
> {
Let's see where this goes, but we may want to rename this slightly to
indicate that this should only be called from interrupt context when
we can't rely on current's audit_context. Bonus points if we can find
a way to enforce this with a WARN() assertion so we can better catch
abuse.
> + struct audit_context *context;
> +
> + if (!audit_ever_enabled)
> + return NULL; /* Return if not auditing. */
> +
> + context = audit_alloc_context(AUDIT_RECORD_CONTEXT);
> + if (!context)
> + return NULL;
> + context->serial = audit_serial();
> + context->ctime = current_kernel_time64();
> + context->in_syscall = 1;
Setting in_syscall is both interesting and a bit troubling, if for no
other reason than I expect most (all?) callers to be in an interrupt
context when audit_alloc_local() is called. Setting in_syscall would
appear to be conceptually in this case. Can you help explain why this
is the right thing to do, or necessary to ensure things are handled
correctly?
Looking quickly at the audit code, it seems to only be used on record
and/or syscall termination to end things properly as well as in some
of the PATH record code paths to limit filename collection to actual
syscalls. However, this was just a quick look so I could be missing
some important points.
> + return context;
> +}
> +
> +void audit_free_context(struct audit_context *context)
> +{
> + if (!context)
> + return;
> audit_free_names(context);
> unroll_tree_refs(context, NULL, 0);
> free_tree_refs(context);
> --
> 1.8.3.1
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak90 (was ghak32) V3 07/10] audit: add support for containerid to network namespaces
From: Paul Moore @ 2018-07-20 22:14 UTC (permalink / raw)
To: rgb
Cc: cgroups, containers, linux-api, linux-audit, linux-fsdevel,
linux-kernel, netdev, ebiederm, luto, jlayton, carlos, dhowells,
viro, simo, Eric Paris, serge
In-Reply-To: <562cfaf7629b64252aac7cf3cecdd70b471af5b5.1528304204.git.rgb@redhat.com>
On Wed, Jun 6, 2018 at 1:03 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Audit events could happen in a network namespace outside of a task
> context due to packets received from the net that trigger an auditing
> rule prior to being associated with a running task. The network
> namespace could in use by multiple containers by association to the
> tasks in that network namespace. We still want a way to attribute
> these events to any potential containers. Keep a list per network
> namespace to track these audit container identifiiers.
>
> Add/increment the audit container identifier on:
> - initial setting of the audit container identifier via /proc
> - clone/fork call that inherits an audit container identifier
> - unshare call that inherits an audit container identifier
> - setns call that inherits an audit container identifier
> Delete/decrement the audit container identifier on:
> - an inherited audit container identifier dropped when child set
> - process exit
> - unshare call that drops a net namespace
> - setns call that drops a net namespace
>
> See: https://github.com/linux-audit/audit-kernel/issues/92
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 23 ++++++++++++++++
> kernel/audit.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
> kernel/auditsc.c | 5 ++++
> kernel/nsproxy.c | 4 +++
> 4 files changed, 104 insertions(+)
...
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 1e37abf..7e2e51c 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -87,6 +88,12 @@ struct audit_field {
> u32 op;
> };
>
> +struct audit_contid {
> + struct list_head list;
> + u64 id;
> + refcount_t refcount;
> +};
Do we need to worry about locking the audit container ID list? Does
the network namespace code (or some other namespace code) ensure that
add/deletes are serialized?
> @@ -156,6 +163,10 @@ extern void audit_log_task_info(struct audit_buffer *ab,
> struct task_struct *tsk);
> extern int audit_log_contid(struct audit_context *context,
> char *op, u64 contid);
> +extern struct list_head *audit_get_contid_list(const struct net *net);
> +extern void audit_contid_add(struct net *net, u64 contid);
> +extern void audit_contid_del(struct net *net, u64 contid);
I wonder if we should change these function names to indicate that
they are managing the netns/cid list? Right now there is no mention
of networking other than the first parameter.
Maybe audit_netns_contid_*()?
> +extern void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p);
>
> extern int audit_update_lsm_rules(void);
>
> @@ -209,6 +220,18 @@ static inline void audit_log_task_info(struct audit_buffer *ab,
> static inline int audit_log_contid(struct audit_context *context,
> char *op, u64 contid)
> { }
> +static inline struct list_head *audit_get_contid_list(const struct net *net)
> +{
> + static LIST_HEAD(list);
> + return &list;
> +}
Why can't we just return NULL here like a normal dummy function? It's
only ever used inside audit. Actually, why is this even in
include/linux/audit.h, couldn't we put it in kernel/audit.h or even
just make it a static in audit.c?
> +static inline void audit_contid_add(struct net *net, u64 contid)
> +{ }
> +static inline void audit_contid_del(struct net *net, u64 contid)
> +{ }
> +static inline void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> +{ }
> +
> #define audit_enabled 0
> #endif /* CONFIG_AUDIT */
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index ba304a8..ecd2de4 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -106,6 +106,7 @@
> */
> struct audit_net {
> struct sock *sk;
> + struct list_head contid_list;
> };
>
> /**
> @@ -311,6 +312,76 @@ static struct sock *audit_get_sk(const struct net *net)
> return aunet->sk;
> }
>
> +/**
> + * audit_get_contid_list - Return the audit container ID list for the given network namespace
> + * @net: the destination network namespace
> + *
> + * Description:
> + * Returns the list pointer if valid, NULL otherwise. The caller must ensure
> + * that a reference is held for the network namespace while the sock is in use.
> + */
> +struct list_head *audit_get_contid_list(const struct net *net)
> +{
> + struct audit_net *aunet = net_generic(net, audit_net_id);
> +
> + return &aunet->contid_list;
> +}
> +
> +void audit_contid_add(struct net *net, u64 contid)
> +{
> + struct list_head *contid_list = audit_get_contid_list(net);
> + struct audit_contid *cont;
> +
> + if (!cid_valid(contid))
> + return;
> + if (!list_empty(contid_list))
> + list_for_each_entry(cont, contid_list, list)
> + if (cont->id == contid) {
> + refcount_inc(&cont->refcount);
> + return;
> + }
<thinking out loud>I think this is fine for right now, but we may need
to be a bit clever about how we store the IDs - walking an unsorted
list with lots of entries may prove to be too painful.</thinking out
loud>
> + cont = kmalloc(sizeof(struct audit_contid), GFP_KERNEL);
> + if (!cont)
> + return;
> + INIT_LIST_HEAD(&cont->list);
> + cont->id = contid;
> + refcount_set(&cont->refcount, 1);
> + list_add(&cont->list, contid_list);
> +}
> +
> +void audit_contid_del(struct net *net, u64 contid)
> +{
> + struct list_head *contid_list = audit_get_contid_list(net);
> + struct audit_contid *cont = NULL;
> + int found = 0;
> +
> + if (!cid_valid(contid))
> + return;
> + if (!list_empty(contid_list))
> + list_for_each_entry(cont, contid_list, list)
> + if (cont->id == contid) {
> + found = 1;
> + break;
You don't really need the found variable, you can just move all of the
work you need to do up into the if statement and return from inside
the if statement.
> + }
> + if (!found)
> + return;
> + list_del(&cont->list);
> + if (refcount_dec_and_test(&cont->refcount))
> + kfree(cont);
Don't you want to dec_and_test first and only remove it from the list
if there are no other references?
> +}
>
> +void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> +{
> + u64 contid = audit_get_contid(p);
> + struct nsproxy *new = p->nsproxy;
> +
> + if (!cid_valid(contid))
> + return;
> + audit_contid_del(ns->net_ns, contid);
> + if (new)
> + audit_contid_add(new->net_ns, contid);
> +}
> +
> void audit_panic(const char *message)
> {
> switch (audit_failure) {
> @@ -1550,6 +1621,7 @@ static int __net_init audit_net_init(struct net *net)
> return -ENOMEM;
> }
> aunet->sk->sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
> + INIT_LIST_HEAD(&aunet->contid_list);
>
> return 0;
> }
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index ea1ee35..6ab5e5e 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -75,6 +75,7 @@
> #include <linux/uaccess.h>
> #include <linux/fsnotify_backend.h>
> #include <uapi/linux/limits.h>
> +#include <net/net_namespace.h>
>
> #include "audit.h"
>
> @@ -2165,6 +2166,7 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> uid_t uid;
> struct tty_struct *tty;
> char comm[sizeof(current->comm)];
> + struct net *net = task->nsproxy->net_ns;
>
> /* Can't set if audit disabled */
> if (!task->audit)
> @@ -2185,10 +2187,13 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> else if (cid_valid(oldcontid) && !task->audit->inherited)
> rc = -EEXIST;
> if (!rc) {
> + if (cid_valid(oldcontid))
> + audit_contid_del(net, oldcontid);
> task_lock(task);
> task->audit->contid = contid;
> task->audit->inherited = false;
> task_unlock(task);
> + audit_contid_add(net, contid);
> }
>
> if (!audit_enabled)
> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> index f6c5d33..dcb69fe 100644
> --- a/kernel/nsproxy.c
> +++ b/kernel/nsproxy.c
> @@ -27,6 +27,7 @@
> #include <linux/syscalls.h>
> #include <linux/cgroup.h>
> #include <linux/perf_event.h>
> +#include <linux/audit.h>
>
> static struct kmem_cache *nsproxy_cachep;
>
> @@ -140,6 +141,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
> struct nsproxy *old_ns = tsk->nsproxy;
> struct user_namespace *user_ns = task_cred_xxx(tsk, user_ns);
> struct nsproxy *new_ns;
> + u64 contid = audit_get_contid(tsk);
>
> if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
> CLONE_NEWPID | CLONE_NEWNET |
> @@ -167,6 +169,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
> return PTR_ERR(new_ns);
>
> tsk->nsproxy = new_ns;
> + audit_contid_add(new_ns->net_ns, contid);
> return 0;
> }
>
> @@ -224,6 +227,7 @@ void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
> ns = p->nsproxy;
> p->nsproxy = new;
> task_unlock(p);
> + audit_switch_task_namespaces(ns, p);
>
> if (ns && atomic_dec_and_test(&ns->count))
> free_nsproxy(ns);
> --
> 1.8.3.1
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
^ 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