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]

Message-Id: <1232417265.3136.36.camel@zeus.themaw.net>
Date:	Tue, 20 Jan 2009 11:07:45 +0900
From:	Ian Kent <raven@...maw.net>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	"Serge E. Hallyn" <serue@...ibm.com>,
	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
Subject: Re: [PATCH] autofs: fix the wrong usage of the deprecated
	task_pgrp_nr()

On Mon, 2009-01-19 at 22:31 +0100, Oleg Nesterov wrote:
> On 01/19, Serge E. Hallyn wrote:
> >
> > Quoting Oleg Nesterov (oleg@...hat.com):
> > > 
> > > This is the next patch. This one does
> > > 
> > > 	--- 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);
> > 
> > Ok, that was the one I had looked at earlier (though now I can't find
> > it).  But that just seems wrong to me.  We should certainly not be
> > caching a pid_vnr in the kernel.  That is imo incomparably worse than
> > storing a pid_nr.
> 
> We do not cache it. We use this pgrp as an argument for find_pid()
> right after return from parse_options(). And find_pid() uses
> current->nsproxy->pid_ns. That is why this is bugfix.
> 
> > Can we just jump straight to caching the struct pid?
> 
> Of course it is ugly to store pid_t and then call find_pid(),
> I don't understand why the code was written this way. But I
> am not going to cleanup this code ;)
> 
> (note also that the 2nd patch I sent for autofs4 does not use
>  pid_t at all).
> 
> > > passing pid_t's in from userspace uses current namespace, with
> > > or without the patch.
> >
> > Which makes sense on the one hand, but OTOH could be confusing
> > if as I requested we print out init_pid_ns values.  (sigh)
> 
> But it is not possible to pass the global pid_t from within
> the subnamespace via "pgrp=" option, automount (or whatever)
> just can't know it when it runs in the subnamespace.
> 
> > Yes... would it be overkill to just print both?
> 
> perharps, I don't know...
> 
> But this is imho a bit off-topic, we can change the debugging
> output later any way we like.

The use of pid_ts in debug statements could be confusing, but generally,
the debug output will only be used when the person gathering it is aware
of the environment it is collected in. We can change these as needed as
time passes.

We also send the pid_t via autofs request packets and it is used for
user space debug prints. It is open to the same confusion and I'm still
not sure how to deal with that but it isn't important to sort that out
now either, for the same reason as above.

Ian


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