linux-kernel - Re: [PATCH] Fix __d_path for lazy unmounts

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Mon, 22 Feb 2010 17:56:07 -0800
From:	John Johansen <john.johansen@...onical.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] Fix __d_path for lazy unmounts

Andrew Morton wrote:
> On Sat, 20 Feb 2010 04:27:38 -0800 john.johansen@...onical.com wrote:
> 
>> From: John Johansen <john.johansen@...onical.com>
>>
>> When __d_path() hits a lazily unmounted mount point, it tries to prepend
>> the name of the lazily unmounted dentry to the path name.  It gets this wrong,
>> and also overwrites the slash that separates the name from the following
>> pathname component. This patch fixes that; if a process was in directory
>> /foo/bar and /foo got lazily unmounted, the old result was ``foobar'' (note the
>> missing slash), while the new result with this patch is ``/foo/bar''.
>>
>> Signed-off-by: John Johansen <john.johansen@...onical.com>
>> ---
>>  fs/dcache.c |   27 +++++++++++++++++++++++----
>>  1 files changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/dcache.c b/fs/dcache.c
>> index 953173a..df49666 100644
>> --- a/fs/dcache.c
>> +++ b/fs/dcache.c
>> @@ -1922,11 +1922,9 @@ char *__d_path(const struct path *path, struct path *root,
>>  	retval = end-1;
>>  	*retval = '/';
>>  
>> -	for (;;) {
>> +	while(dentry != root->dentry || vfsmnt != root->mnt) {
> 
thanks, forgot to refresh after checkpatch

> Please put a space between the `while' and the `('.
> 
>>  		struct dentry * parent;
>>  
>> -		if (dentry == root->dentry && vfsmnt == root->mnt)
>> -			break;
>>  		if (dentry == vfsmnt->mnt_root || IS_ROOT(dentry)) {
>>  			/* Global root? */
>>  			if (vfsmnt->mnt_parent == vfsmnt) {
>> @@ -1950,9 +1948,30 @@ out:
>>  	return retval;
>>  
>>  global_root:
>> -	retval += 1;	/* hit the slash */
>> +	/*
>> +	 * We went past the (vfsmount, dentry) we were looking for and have
>> +	 * either hit a root dentry, a lazily unmounted dentry, an
>> +	 * unconnected dentry, or the file is on a pseudo filesystem.
>> +	 */
>> +	if ((dentry->d_sb->s_flags & MS_NOUSER) ||
>> +	    (dentry->d_name.len = 1 && *dentry->d_name.name == '/')) {
> 
> Did you really mean to assign 1 to dentry->d_name.len here?  Was `=='
> intended?  I hope so, because modifying the dentry in d_path() would be odd.
> 
> If this was a mistake then why did the patch pass testing?
> 
Nope, definite bug, missed that case in testing.  In this case every test that
had a leading '/' had a d_name.len == 1 as well.

I haven't seen the case of where a root dentry has a leading / and doesn't have
a d_name.len == 1 and if that case never happens the test wouldn't be needed.

I will respin the patch, and include testing this time

thanks
john
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/