linux-kernel - Re: [PATCH] autofs: fix the wrong usage of the deprecated task_pgrp

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:	Fri, 23 Jan 2009 13:48:20 +0900
From:	Ian Kent <raven@...maw.net>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>, hpa@...or.com,
	Pavel Emelyanov <xemul@...nvz.org>,
	Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>,
	linux-kernel@...r.kernel.org, "Serge E. Hallyn" <serue@...ibm.com>
Subject: Re: [PATCH] autofs: fix the wrong usage of the deprecated
	task_pgrp_nr()

On Sun, 2009-01-18 at 08:34 +0100, Oleg Nesterov wrote:
> parse_options(&pgid) sets pgid = task_pgrp_nr() which uses the global
> namespace. This is wrong, we use this pgid to find "struct pid" in the
> current's namespace. Change parse_options() to use task_pgrp_vnr().
> 
> Also do s/task_pgrp_nr/task_pgrp_vnr/ in the debugging printks.
> checkpatch.pl complains about "line over 80 characters", but it should
> blame the cuurent code, not the patch.

Following the lengthy discussion, largely for my benefit, I'm now
looking at the patch with some idea of sensibility.

It seems to me that when setting sbi->oz_pgrp we should use
task_pgrp_vnr(). As Oleg says, and I agree, this should always give
correct results when checking if current is the session leader.

But I think that for debug logging we should always report numbers that
correspond to the object as it is seen in the global namespace as that
is the most likely reference point someone examining debug logging will
be working from.

Oleg, what should be the recommended call to report that, instead of the
task_pgrp_nr() call?

> 
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>
> 
> --- CUR/fs/autofs/inode.c~1_AUTOFS	2009-01-12 23:07:46.000000000 +0100
> +++ CUR/fs/autofs/inode.c	2009-01-18 06:18:49.000000000 +0100
> @@ -78,7 +78,7 @@ static int parse_options(char *options, 
>  
>  	*uid = current_uid();
>  	*gid = current_gid();
> -	*pgrp = task_pgrp_nr(current);
> +	*pgrp = task_pgrp_vnr(current);
>  
>  	*minproto = *maxproto = AUTOFS_PROTO_VERSION;
>  
> --- CUR/fs/autofs/root.c~1_AUTOFS	2008-10-10 00:13:53.000000000 +0200
> +++ CUR/fs/autofs/root.c	2009-01-18 06:15:42.000000000 +0100
> @@ -215,7 +215,7 @@ static struct dentry *autofs_root_lookup
>  	oz_mode = autofs_oz_mode(sbi);
>  	DPRINTK(("autofs_lookup: pid = %u, pgrp = %u, catatonic = %d, "
>  				"oz_mode = %d\n", task_pid_nr(current),
> -				task_pgrp_nr(current), sbi->catatonic,
> +				task_pgrp_vnr(current), sbi->catatonic,
>  				oz_mode));
>  
>  	/*
> @@ -550,7 +550,8 @@ static int autofs_root_ioctl(struct inod
>  	struct autofs_sb_info *sbi = autofs_sbi(inode->i_sb);
>  	void __user *argp = (void __user *)arg;
>  
> -	DPRINTK(("autofs_ioctl: cmd = 0x%08x, arg = 0x%08lx, sbi = %p, pgrp = %u\n",cmd,arg,sbi,task_pgrp_nr(current)));
> +	DPRINTK(("autofs_ioctl: cmd = 0x%08x, arg = 0x%08lx, sbi = %p, pgrp = %u\n",
> +		cmd, arg, sbi, task_pgrp_vnr(current)));
>  
>  	if (_IOC_TYPE(cmd) != _IOC_TYPE(AUTOFS_IOC_FIRST) ||
>  	     _IOC_NR(cmd) - _IOC_NR(AUTOFS_IOC_FIRST) >= AUTOFS_IOC_COUNT)
> 

--
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/