linux-kernel - Re: [PATCH 09/23] ns: Introduce ns_idr to be able to iterate all allocated namespaces in the system

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 30 Jul 2020 16:32:22 +0300
From:   Kirill Tkhai <ktkhai@...tuozzo.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     viro@...iv.linux.org.uk, adobriyan@...il.com, davem@...emloft.net,
        ebiederm@...ssion.com, akpm@...ux-foundation.org,
        christian.brauner@...ntu.com, areber@...hat.com, serge@...lyn.com,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 09/23] ns: Introduce ns_idr to be able to iterate all
 allocated namespaces in the system

On 30.07.2020 15:23, Matthew Wilcox wrote:
> On Thu, Jul 30, 2020 at 03:00:08PM +0300, Kirill Tkhai wrote:
>> This patch introduces a new IDR and functions to add/remove and iterate
>> registered namespaces in the system. It will be used to list namespaces
>> in /proc/namespaces/... in next patches.
> 
> Looks like you could use an XArray for this and it would be fewer lines of
> code.
> 
>>  
>>  static struct vfsmount *nsfs_mnt;
>> +static DEFINE_SPINLOCK(ns_lock);
>> +static DEFINE_IDR(ns_idr);
> 
> XArray includes its own spinlock.
> 
>> +/*
>> + * Add a newly created ns to ns_idr. The ns must be fully
>> + * initialized since it becomes available for ns_get_next()
>> + * right after we exit this function.
>> + */
>> +int ns_idr_register(struct ns_common *ns)
>> +{
>> +	int ret, id = ns->inum - PROC_NS_MIN_INO;
>> +
>> +	if (WARN_ON(id < 0))
>> +		return -EINVAL;
>> +
>> +	idr_preload(GFP_KERNEL);
>> +	spin_lock_irq(&ns_lock);
>> +	ret = idr_alloc(&ns_idr, ns, id, id + 1, GFP_ATOMIC);
>> +	spin_unlock_irq(&ns_lock);
>> +	idr_preload_end();
>> +	return ret < 0 ? ret : 0;
> 
> This would simply be return xa_insert_irq(...);
> 
>> +}
>> +
>> +/*
>> + * Remove a dead ns from ns_idr. Note, that ns memory must
>> + * be freed not earlier then one RCU grace period after
>> + * this function, since ns_get_next() uses RCU to iterate the IDR.
>> + */
>> +void ns_idr_unregister(struct ns_common *ns)
>> +{
>> +	int id = ns->inum - PROC_NS_MIN_INO;
>> +	unsigned long flags;
>> +
>> +	if (WARN_ON(id < 0))
>> +		return;
>> +
>> +	spin_lock_irqsave(&ns_lock, flags);
>> +	idr_remove(&ns_idr, id);
>> +	spin_unlock_irqrestore(&ns_lock, flags);
>> +}
> 
> xa_erase_irqsave();

static inline void *xa_erase_irqsave(struct xarray *xa, unsigned long index)
{
	unsigned long flags;
        void *entry;

        xa_lock_irqsave(xa, flags);
        entry = __xa_erase(xa, index);
        xa_unlock_irqrestore(xa, flags);

        return entry;
}

>> +
>> +/*
>> + * This returns ns with inum greater than @id or NULL.
>> + * @id is updated to refer the ns inum.
>> + */
>> +struct ns_common *ns_get_next(unsigned int *id)
>> +{
>> +	struct ns_common *ns;
>> +
>> +	if (*id < PROC_NS_MIN_INO - 1)
>> +		*id = PROC_NS_MIN_INO - 1;
>> +
>> +	*id += 1;
>> +	*id -= PROC_NS_MIN_INO;
>> +
>> +	rcu_read_lock();
>> +	do {
>> +		ns = idr_get_next(&ns_idr, id);
>> +		if (!ns)
>> +			break;
> 
> xa_find_after();
> 
> You'll want a temporary unsigned long to work with ...
> 
>> +		if (!refcount_inc_not_zero(&ns->count)) {
>> +			ns = NULL;
>> +			*id += 1;
> 
> you won't need this increment.

Why? I don't see a way xarray allows to avoid this.