Re: [PATCH RFC] namei: don't drop link paths acquired under LOOKUP_RCU

[Jens Axboe <axboe@kernel.dk>]

On 2/14/21 9:40 AM, Al Viro wrote:
> On Sun, Feb 14, 2021 at 04:05:22PM +0000, Al Viro wrote:
>> On Sun, Feb 07, 2021 at 01:26:19PM -0700, Jens Axboe wrote:
>>
>>> Al, not sure if this is the right fix for the situation, but it's
>>> definitely a problem. Observed by doing a LOOKUP_CACHED of something with
>>> links, using /proc/self/comm as the example in the attached way to
>>> demonstrate this problem.
>>
>> That's definitely not the right fix.  What your analysis has missed is
>> what legitimize_links() does to nd->depth when called.  IOW, on transitions
>> from RCU mode you want nd->depth to set according the number of links we'd
>> grabbed references to.  Flatly setting it to 0 on failure exit will lead
>> to massive leaks.
>>
>> Could you check if the following fixes your reproducers?
>>
>> diff --git a/fs/namei.c b/fs/namei.c
>> index 4cae88733a5c..afb293b39be7 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -687,7 +687,7 @@ static bool try_to_unlazy(struct nameidata *nd)
>>  
>>  	nd->flags &= ~LOOKUP_RCU;
>>  	if (nd->flags & LOOKUP_CACHED)
>> -		goto out1;
>> +		goto out2;
>>  	if (unlikely(!legitimize_links(nd)))
>>  		goto out1;
>>  	if (unlikely(!legitimize_path(nd, &nd->path, nd->seq)))
>> @@ -698,6 +698,8 @@ static bool try_to_unlazy(struct nameidata *nd)
>>  	BUG_ON(nd->inode != parent->d_inode);
>>  	return true;
>>  
>> +out2:
>> +	nd->depth = 0;	// as we hadn't gotten to legitimize_links()
>>  out1:
>>  	nd->path.mnt = NULL;
>>  	nd->path.dentry = NULL;
>> @@ -725,7 +727,7 @@ static bool try_to_unlazy_next(struct nameidata *nd, struct dentry *dentry, unsi
>>  
>>  	nd->flags &= ~LOOKUP_RCU;
>>  	if (nd->flags & LOOKUP_CACHED)
>> -		goto out2;
>> +		goto out3;
>>  	if (unlikely(!legitimize_links(nd)))
>>  		goto out2;
>>  	if (unlikely(!legitimize_mnt(nd->path.mnt, nd->m_seq)))
>> @@ -753,6 +755,8 @@ static bool try_to_unlazy_next(struct nameidata *nd, struct dentry *dentry, unsi
>>  	rcu_read_unlock();
>>  	return true;
>>  
>> +out3:
>> +	nd->depth = 0;	// as we hadn't gotten to legitimize_links()
>>  out2:
>>  	nd->path.mnt = NULL;
>>  out1:
> 
> Alternatively, we could use the fact that legitimize_links() is not
> called anywhere other than these two places and have LOOKUP_CACHED
> checked there.  As in

Both fix the issue for me, just tested them. The second one seems
cleaner to me, would probably be nice to have a comment on that in
either the two callers or at least in legitimize_links() though.

-- 
Jens Axboe