RE: [PATCH] namei: revert old behaviour for filename_lookup with LOOKUP_PARENT flag

[Remanan Pillai, Vineeth]

Thanks for the detailed explanation. I understand the reasoning now. 

Thanks, 
Vineeth 
________________________________________
From: Al Viro [viro@xxxxxxxxxxxxxxxx] on behalf of Al Viro [viro@xxxxxxxxxxxxxxxxxx]
Sent: Thursday, October 13, 2016 4:31 PM
To: Remanan Pillai, Vineeth
Cc: Christoph Hellwig; linux-fsdevel@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Kamata, Munehisa; Liguori, Anthony
Subject: Re: [PATCH] namei: revert old behaviour for filename_lookup with LOOKUP_PARENT flag

On Thu, Oct 13, 2016 at 03:14:23PM -0700, Vineeth Remanan Pillai wrote:
> Unfortunately, I also do not have enough context about the customer
> code that uses it. Since kern_path was exported function and the
> behavior changed across releases, this patch is just trying to revert
> to the old behavior.
> I clearly get what you are trying to say. It would have been really
> beneficial to get the context so as to understand use case and figure out
> alternative or better approach. But I think we should have the functionality
> as before and if kern_path is not the right API for this purpose, probably
> we
> should deprecate it in a phased manner.

kern_path() is not the right API for this purpose.  Never had been.  It is,
OTOH, the right API for other purposes, so it's not going to disappear.
If you check the history of mainline tree, you'll see no such call sites.
All way back to 2008 when kern_path() had been introduced, it had never
been given LOOKUP_PARENT in arguments:

; git log -p -Skern_path | grep ^\+.*\<kern_path\>
+       err = kern_path(mtd_dev, LOOKUP_FOLLOW, &path);
+       error = kern_path(pathname, LOOKUP_FOLLOW, &path);
+                               ret = kern_path(filename, LOOKUP_FOLLOW, &path);
+       ret = kern_path(pathname->name, LOOKUP_FOLLOW, &path);
+       err = kern_path(name, LOOKUP_FOLLOW, path);
+               error = kern_path(journal_path, LOOKUP_FOLLOW, &path);
+                       error = kern_path(journal_path, LOOKUP_FOLLOW, &path);
+               rc = kern_path(name, LOOKUP_FOLLOW, &path);
+       ret = kern_path(filename, LOOKUP_FOLLOW, &path);
+               status = kern_path(nfs4_recoverydir(), LOOKUP_FOLLOW, &path);
+static struct ctl_path kern_path[] = { { .procname = "kernel", }, { } };
+       register_sysctl_paths(kern_path, pid_ns_ctl_table);
+               r = kern_path(syspath, LOOKUP_FOLLOW, &path);
+       status = kern_path(recdir, LOOKUP_FOLLOW, &path);
+       /* Drop refcount obtained by kern_path(). */
+       rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path);
+               ecryptfs_printk(KERN_WARNING, "kern_path() failed\n");
+       if (kern_path(tomoyo_loader, LOOKUP_FOLLOW, &path)) {
+               if (kern_path(dev_name, LOOKUP_FOLLOW, &path)) {
+       err = kern_path(mtd_dev, LOOKUP_FOLLOW, &path);
+       ret = kern_path(cache->rootdirname, LOOKUP_DIRECTORY, &path);
+       error = kern_path(pathname, LOOKUP_FOLLOW, &path);
+       err = kern_path(mountpoint, LOOKUP_FOLLOW, &path);
+       int err = kern_path(pathname, 0, &path);
+                       err = kern_path(name, LOOKUP_FOLLOW, &path);
+       error = kern_path(name, LOOKUP_FOLLOW, &path);
+       error = kern_path(dev_name, LOOKUP_FOLLOW, &path);
+       if (kern_path(tomoyo_loader, LOOKUP_FOLLOW, &path)) {
+       if (pathname && kern_path(pathname, LOOKUP_FOLLOW, &path) == 0) {
+       if (pathname && kern_path(pathname, 0, &path) == 0) {
+               err = kern_path(tree->pathname, 0, &path);
+       err = kern_path(tree->pathname, 0, &path);
+       err = kern_path(new, 0, &path);
+       err = kern_path(old, 0, &path);
+               err = kern_path(tree->pathname, 0, &path);
+       error = kern_path(pathname, LOOKUP_FOLLOW, &path);
+       ret = kern_path(symname, LOOKUP_FOLLOW|LOOKUP_DIRECTORY, path);
+       rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path);
+               err = kern_path(sunname->sun_path, LOOKUP_FOLLOW, &path);
+               err = kern_path(buf, 0, &key.ek_path);
+       err = kern_path(nxp->ex_path, 0, &path);
+       err = kern_path(nxp->ex_path, 0, &path);
+       if (kern_path(name, 0, &path)) {
+       status = kern_path(rec_dirname, LOOKUP_FOLLOW | LOOKUP_DIRECTORY,
+       status = kern_path(recdir, LOOKUP_FOLLOW, &path);
+       error = kern_path(fo_path, 0, &path);
+       err = kern_path(buf, 0, &exp.ex_path);
+       error = kern_path(name, LOOKUP_FOLLOW, &path);
+       err = kern_path(name, LOOKUP_FOLLOW, &path);
+       err = kern_path(name, LOOKUP_FOLLOW, &path);
+       err = kern_path(name, LOOKUP_FOLLOW, &path);
+       err = kern_path(old_name, LOOKUP_FOLLOW, &old_path);
+       err = kern_path(old_name, LOOKUP_FOLLOW, &old_path);
+       retval = kern_path(dir_name, LOOKUP_FOLLOW, &path);
+int kern_path(const char *name, unsigned int flags, struct path *path)
+EXPORT_SYMBOL(kern_path);
+extern int kern_path(const char *, unsigned, struct path *);
;

As you can see, it had only been given LOOKUP_FOLLOW | LOOKUP_DIRECTORY,
LOOKUP_FOLLOW or 0.  Old behaviour in a sense of kern_path() accepting
LOOKUP_PARENT is not going to come back - it would have been inherently racy
all along *and* nothing in mainline kernel had ever attempted that stupidity.

LOOKUP_PARENT had always been only for primitives that had left nameidata
to the caller, so that it could do something about the last component.
kern_path() is nothing of that kind and no, I'm not going to reexpose
nameidata to anything outside of fs/namei.c.

Out of existing primitives kern_path_locked() is the closest sane
approximation.  It could be exported, if your customer cares to give
details.  If they do not, tell them that their abuse of kern_path() accidental
behaviour had been wrong all along and the old versions of their out-of-tree
code are almost certainly racy.  If they are interested in safe replacement,
they'd better provide details.

BTW, see 15a9155fe3e8 (fix race in audit_get_nd()) for an example of similar
race.  That was the one and only bug of that sort that made it into the
mainline.  That was path_lookup() with LOOKUP_PARENT (in principle legitimate)
with nd simply discarded by the caller.