Date:   Fri, 20 Jul 2018 16:58:40 -0500
From:   Tyler Hicks <tyhicks@...onical.com>
To:     Christian Brauner <christian@...uner.io>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Tejun Heo <tj@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        netdev@...r.kernel.org,
        Linux Containers <containers@...ts.linux-foundation.org>,
        bridge@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
        "Eric W. Biederman" <ebiederm@...ssion.com>
Subject: Re: [PATCH net-next v2 6/7] net: Create reusable function for getting
 ownership info of sysfs inodes

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@...onical.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@...ts.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/containers




Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists