LinuxLists.cc - [PATCH v2 net-next 0/7] Make /sys/class/net per net namespace objects belong to container

2018-07-18 04:18:28

by David Miller

[permalink] [raw]

Subject: Re: [PATCH v2 net-next 0/7] Make /sys/class/net per net namespace objects belong to container

From: Tyler Hicks <[email protected]>
Date: Fri, 13 Jul 2018 16:05:42 +0000

> 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.
>
> I've verified that all of the bridge related files affected by patch 7
> have proper access control checks for CAP_NET_ADMIN inside of the
> user namespace. I have *not* yet verified that all of the network
> device related sysfs files affected by patch 5 have proper access
> control checks. I was working under the assumption that those code paths
> already were verified when the first iteration of the patches were sent
> out.

Ok, I can't let this series rot forever, so I'll apply it to net-next.

Thank you.

2018-07-18 04:43:22

by David Miller

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 <[email protected]>
>> ---
>> 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
>> [email protected]
>> https://lists.linuxfoundation.org/mailman/listinfo/containers

Attachments:

signature.asc (849.00 B)
OpenPGP digital signature