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 [v...@ftp.linux.org.uk] on behalf of Al Viro 
[v...@zeniv.linux.org.uk]
Sent: Thursday, October 13, 2016 4:31 PM
To: Remanan Pillai, Vineeth
Cc: Christoph Hellwig; linux-fsde...@vger.kernel.org; 
linux-kernel@vger.kernel.org; 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, );
+   error = kern_path(pathname, LOOKUP_FOLLOW, );
+   ret = kern_path(filename, LOOKUP_FOLLOW, );
+   ret = kern_path(pathname->name, LOOKUP_FOLLOW, );
+   err = kern_path(name, LOOKUP_FOLLOW, path);
+   error = kern_path(journal_path, LOOKUP_FOLLOW, );
+   error = kern_path(journal_path, LOOKUP_FOLLOW, );
+   rc = kern_path(name, LOOKUP_FOLLOW, );
+   ret = kern_path(filename, LOOKUP_FOLLOW, );
+   status = kern_path(nfs4_recoverydir(), LOOKUP_FOLLOW, );
+static struct ctl_path kern_path[] = { { .procname = "kernel", }, { } };
+   register_sysctl_paths(kern_path, pid_ns_ctl_table);
+   r = kern_path(syspath, LOOKUP_FOLLOW, );
+   status = kern_path(recdir, LOOKUP_FOLLOW, );
+   /* Drop refcount obtained by kern_path(). */
+   rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, );
+   ecryptfs_printk(KERN_WARNING, "kern_path() failed\n");
+   if (kern_path(tomoyo_loader, LOOKUP_FOLLOW, )) {
+   if (kern_path(dev_name, LOOKUP_FOLLOW, )) {
+   err = kern_path(mtd_dev, LOOKUP_FOLLOW, );
+   ret = kern_path(cache->rootdirname, LOOKUP_DIRECTORY, );
+   error = kern_path(pathname, LOOKUP_FOLLOW, );
+   err = kern_path(mountpoint, LOOKUP_FOLLOW, );
+   int err = kern_path(pathname, 0, );
+   err = kern_path(name, LOOKUP_FOLLOW, );
+   error = kern_path(name, LOOKUP_FOLLOW, );
+   error = kern_path(dev_name, LOOKUP_FOLLOW, );
+   if (kern_path(tomoyo_loader, LOOKUP_FOLLOW, )) {
+   if (pathname && kern_path(pathname, LOOKUP_FOLLOW, ) == 0) {
+   if (pathname && kern_path(pathname, 0, ) == 0) {
+   err = kern_path(tree->pathname, 0, );
+   err = kern_path(tree->pathname, 0, );
+   err = kern_path(new, 0, );
+   err = kern_path(old, 0, );
+   err = kern_path(tree->pathname, 0, );
+   error = kern_path(pathname, LOOKUP_FOLLOW, );
+   ret = kern_path(symname, LOOKUP_FOLLOW|LOOKUP_DIRECTORY, path);
+   rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, );
+   err = kern_path(sunname->sun_path, LOOKUP_FOLLOW, );
+   err = kern_path(buf, 0, _path);
+   err = kern_path(nxp->ex_path, 0, );
+   err = kern_path(nxp->ex_path, 0, );
+   if (kern_path(name, 0, )) {
+   status = kern_path(rec_dirname, LOOKUP_FOLLOW | LOOKUP_DIRECTORY,
+   status = kern_path(recdir, LOOKUP_FOLLOW, );
+   error = kern_path(fo_path, 0, );
+   err = kern_path(buf, 0, _path);
+   error = kern_path(name, LOOKUP_FOLLOW, );
+   err = kern_path(name, LOOKUP_FOLLOW, );
+   err = kern_path(name, LOOKUP_FOLLOW, );
+   err = kern_path(name, LOOKUP_FOLLOW, );
+   err = kern_path(old_name, LOOKUP_FOLLOW, _path);
+   err = kern_path(old_name, LOOKUP_FOLLOW, _path);
+   retval = kern_path(dir_name, LOOKUP_FOLLOW, );
+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 co

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 [v...@ftp.linux.org.uk] on behalf of Al Viro 
[v...@zeniv.linux.org.uk]
Sent: Thursday, October 13, 2016 4:31 PM
To: Remanan Pillai, Vineeth
Cc: Christoph Hellwig; linux-fsde...@vger.kernel.org; 
linux-kernel@vger.kernel.org; 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 ^\+.*\
+   err = kern_path(mtd_dev, LOOKUP_FOLLOW, );
+   error = kern_path(pathname, LOOKUP_FOLLOW, );
+   ret = kern_path(filename, LOOKUP_FOLLOW, );
+   ret = kern_path(pathname->name, LOOKUP_FOLLOW, );
+   err = kern_path(name, LOOKUP_FOLLOW, path);
+   error = kern_path(journal_path, LOOKUP_FOLLOW, );
+   error = kern_path(journal_path, LOOKUP_FOLLOW, );
+   rc = kern_path(name, LOOKUP_FOLLOW, );
+   ret = kern_path(filename, LOOKUP_FOLLOW, );
+   status = kern_path(nfs4_recoverydir(), LOOKUP_FOLLOW, );
+static struct ctl_path kern_path[] = { { .procname = "kernel", }, { } };
+   register_sysctl_paths(kern_path, pid_ns_ctl_table);
+   r = kern_path(syspath, LOOKUP_FOLLOW, );
+   status = kern_path(recdir, LOOKUP_FOLLOW, );
+   /* Drop refcount obtained by kern_path(). */
+   rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, );
+   ecryptfs_printk(KERN_WARNING, "kern_path() failed\n");
+   if (kern_path(tomoyo_loader, LOOKUP_FOLLOW, )) {
+   if (kern_path(dev_name, LOOKUP_FOLLOW, )) {
+   err = kern_path(mtd_dev, LOOKUP_FOLLOW, );
+   ret = kern_path(cache->rootdirname, LOOKUP_DIRECTORY, );
+   error = kern_path(pathname, LOOKUP_FOLLOW, );
+   err = kern_path(mountpoint, LOOKUP_FOLLOW, );
+   int err = kern_path(pathname, 0, );
+   err = kern_path(name, LOOKUP_FOLLOW, );
+   error = kern_path(name, LOOKUP_FOLLOW, );
+   error = kern_path(dev_name, LOOKUP_FOLLOW, );
+   if (kern_path(tomoyo_loader, LOOKUP_FOLLOW, )) {
+   if (pathname && kern_path(pathname, LOOKUP_FOLLOW, ) == 0) {
+   if (pathname && kern_path(pathname, 0, ) == 0) {
+   err = kern_path(tree->pathname, 0, );
+   err = kern_path(tree->pathname, 0, );
+   err = kern_path(new, 0, );
+   err = kern_path(old, 0, );
+   err = kern_path(tree->pathname, 0, );
+   error = kern_path(pathname, LOOKUP_FOLLOW, );
+   ret = kern_path(symname, LOOKUP_FOLLOW|LOOKUP_DIRECTORY, path);
+   rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, );
+   err = kern_path(sunname->sun_path, LOOKUP_FOLLOW, );
+   err = kern_path(buf, 0, _path);
+   err = kern_path(nxp->ex_path, 0, );
+   err = kern_path(nxp->ex_path, 0, );
+   if (kern_path(name, 0, )) {
+   status = kern_path(rec_dirname, LOOKUP_FOLLOW | LOOKUP_DIRECTORY,
+   status = kern_path(recdir, LOOKUP_FOLLOW, );
+   error = kern_path(fo_path, 0, );
+   err = kern_path(buf, 0, _path);
+   error = kern_path(name, LOOKUP_FOLLOW, );
+   err = kern_path(name, LOOKUP_FOLLOW, );
+   err = kern_path(name, LOOKUP_FOLLOW, );
+   err = kern_path(name, LOOKUP_FOLLOW, );
+   err = kern_path(old_name, LOOKUP_FOLLOW, _path);
+   err = kern_path(old_name, LOOKUP_FOLLOW, _path);
+   retval = kern_path(dir_name, LOOKUP_FOLLOW, );
+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 wou

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

[Al Viro]

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 ^\+.*\
+   err = kern_path(mtd_dev, LOOKUP_FOLLOW, );
+   error = kern_path(pathname, LOOKUP_FOLLOW, );
+   ret = kern_path(filename, LOOKUP_FOLLOW, );
+   ret = kern_path(pathname->name, LOOKUP_FOLLOW, );
+   err = kern_path(name, LOOKUP_FOLLOW, path);
+   error = kern_path(journal_path, LOOKUP_FOLLOW, );
+   error = kern_path(journal_path, LOOKUP_FOLLOW, );
+   rc = kern_path(name, LOOKUP_FOLLOW, );
+   ret = kern_path(filename, LOOKUP_FOLLOW, );
+   status = kern_path(nfs4_recoverydir(), LOOKUP_FOLLOW, );
+static struct ctl_path kern_path[] = { { .procname = "kernel", }, { } };
+   register_sysctl_paths(kern_path, pid_ns_ctl_table);
+   r = kern_path(syspath, LOOKUP_FOLLOW, );
+   status = kern_path(recdir, LOOKUP_FOLLOW, );
+   /* Drop refcount obtained by kern_path(). */
+   rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, );
+   ecryptfs_printk(KERN_WARNING, "kern_path() failed\n");
+   if (kern_path(tomoyo_loader, LOOKUP_FOLLOW, )) {
+   if (kern_path(dev_name, LOOKUP_FOLLOW, )) {
+   err = kern_path(mtd_dev, LOOKUP_FOLLOW, );
+   ret = kern_path(cache->rootdirname, LOOKUP_DIRECTORY, );
+   error = kern_path(pathname, LOOKUP_FOLLOW, );
+   err = kern_path(mountpoint, LOOKUP_FOLLOW, );
+   int err = kern_path(pathname, 0, );
+   err = kern_path(name, LOOKUP_FOLLOW, );
+   error = kern_path(name, LOOKUP_FOLLOW, );
+   error = kern_path(dev_name, LOOKUP_FOLLOW, );
+   if (kern_path(tomoyo_loader, LOOKUP_FOLLOW, )) {
+   if (pathname && kern_path(pathname, LOOKUP_FOLLOW, ) == 0) {
+   if (pathname && kern_path(pathname, 0, ) == 0) {
+   err = kern_path(tree->pathname, 0, );
+   err = kern_path(tree->pathname, 0, );
+   err = kern_path(new, 0, );
+   err = kern_path(old, 0, );
+   err = kern_path(tree->pathname, 0, );
+   error = kern_path(pathname, LOOKUP_FOLLOW, );
+   ret = kern_path(symname, LOOKUP_FOLLOW|LOOKUP_DIRECTORY, path);
+   rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, );
+   err = kern_path(sunname->sun_path, LOOKUP_FOLLOW, );
+   err = kern_path(buf, 0, _path);
+   err = kern_path(nxp->ex_path, 0, );
+   err = kern_path(nxp->ex_path, 0, );
+   if (kern_path(name, 0, )) {
+   status = kern_path(rec_dirname, LOOKUP_FOLLOW | LOOKUP_DIRECTORY,
+   status = kern_path(recdir, LOOKUP_FOLLOW, );
+   error = kern_path(fo_path, 0, );
+   err = kern_path(buf, 0, _path);
+   error = kern_path(name, LOOKUP_FOLLOW, );
+   err = kern_path(name, LOOKUP_FOLLOW, );
+   err = kern_path(name, LOOKUP_FOLLOW, );
+   err = kern_path(name, LOOKUP_FOLLOW, );
+   err = kern_path(old_name, LOOKUP_FOLLOW, _path);
+   err = kern_path(old_name, LOOKUP_FOLLOW, _path);
+   retval = kern_path(dir_name, LOOKUP_FOLLOW, );
+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

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

[Al Viro]

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 ^\+.*\
+   err = kern_path(mtd_dev, LOOKUP_FOLLOW, );
+   error = kern_path(pathname, LOOKUP_FOLLOW, );
+   ret = kern_path(filename, LOOKUP_FOLLOW, );
+   ret = kern_path(pathname->name, LOOKUP_FOLLOW, );
+   err = kern_path(name, LOOKUP_FOLLOW, path);
+   error = kern_path(journal_path, LOOKUP_FOLLOW, );
+   error = kern_path(journal_path, LOOKUP_FOLLOW, );
+   rc = kern_path(name, LOOKUP_FOLLOW, );
+   ret = kern_path(filename, LOOKUP_FOLLOW, );
+   status = kern_path(nfs4_recoverydir(), LOOKUP_FOLLOW, );
+static struct ctl_path kern_path[] = { { .procname = "kernel", }, { } };
+   register_sysctl_paths(kern_path, pid_ns_ctl_table);
+   r = kern_path(syspath, LOOKUP_FOLLOW, );
+   status = kern_path(recdir, LOOKUP_FOLLOW, );
+   /* Drop refcount obtained by kern_path(). */
+   rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, );
+   ecryptfs_printk(KERN_WARNING, "kern_path() failed\n");
+   if (kern_path(tomoyo_loader, LOOKUP_FOLLOW, )) {
+   if (kern_path(dev_name, LOOKUP_FOLLOW, )) {
+   err = kern_path(mtd_dev, LOOKUP_FOLLOW, );
+   ret = kern_path(cache->rootdirname, LOOKUP_DIRECTORY, );
+   error = kern_path(pathname, LOOKUP_FOLLOW, );
+   err = kern_path(mountpoint, LOOKUP_FOLLOW, );
+   int err = kern_path(pathname, 0, );
+   err = kern_path(name, LOOKUP_FOLLOW, );
+   error = kern_path(name, LOOKUP_FOLLOW, );
+   error = kern_path(dev_name, LOOKUP_FOLLOW, );
+   if (kern_path(tomoyo_loader, LOOKUP_FOLLOW, )) {
+   if (pathname && kern_path(pathname, LOOKUP_FOLLOW, ) == 0) {
+   if (pathname && kern_path(pathname, 0, ) == 0) {
+   err = kern_path(tree->pathname, 0, );
+   err = kern_path(tree->pathname, 0, );
+   err = kern_path(new, 0, );
+   err = kern_path(old, 0, );
+   err = kern_path(tree->pathname, 0, );
+   error = kern_path(pathname, LOOKUP_FOLLOW, );
+   ret = kern_path(symname, LOOKUP_FOLLOW|LOOKUP_DIRECTORY, path);
+   rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, );
+   err = kern_path(sunname->sun_path, LOOKUP_FOLLOW, );
+   err = kern_path(buf, 0, _path);
+   err = kern_path(nxp->ex_path, 0, );
+   err = kern_path(nxp->ex_path, 0, );
+   if (kern_path(name, 0, )) {
+   status = kern_path(rec_dirname, LOOKUP_FOLLOW | LOOKUP_DIRECTORY,
+   status = kern_path(recdir, LOOKUP_FOLLOW, );
+   error = kern_path(fo_path, 0, );
+   err = kern_path(buf, 0, _path);
+   error = kern_path(name, LOOKUP_FOLLOW, );
+   err = kern_path(name, LOOKUP_FOLLOW, );
+   err = kern_path(name, LOOKUP_FOLLOW, );
+   err = kern_path(name, LOOKUP_FOLLOW, );
+   err = kern_path(old_name, LOOKUP_FOLLOW, _path);
+   err = kern_path(old_name, LOOKUP_FOLLOW, _path);
+   retval = kern_path(dir_name, LOOKUP_FOLLOW, );
+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 

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

[Vineeth Remanan Pillai]

On 10/13/2016 02:24 PM, Al Viro wrote:

On Thu, Oct 13, 2016 at 01:41:11PM -0700, Vineeth Remanan Pillai wrote:


Yes, the use case is out-of-tree and the code snippet above depicts the use
.
Since kern_path_locked is also not exported, out-of-tree code used kern_path
for the existence check for directories.

One reference about this issue can be seen here.

http://www.gossamer-threads.com/lists/linux/kernel/2459690?do=post_view_flat#2459690

... and in that thread I have asked for details and got no reply whatsoever.
  

We also have a customer who complained about this functionality change.

I understand that there has been no API promises been made to this API. But
since this is an
exported function, the change in function could cause break in out-of-tree
kernel code. I will
rephrase the commit message to say "change in functionality" instead of
regression

In principle, I have no strong objections against exporting kern_path_locked,
provided it really matches what they (whoever they are) need.  I'm still
curious about the context, though - what is that code trying to do?  Depending
on the actual stuff it wants to implement, there might be better primitives
for doing that *and* there might be something worth adding and exporting
that would be a better match.

It's not that kern_path_locked() isn't a sane interface, but... using it
might be a sign of trying to work around something missing in API.  So again,
please post more details.

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.

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

[Vineeth Remanan Pillai]

On 10/13/2016 02:24 PM, Al Viro wrote:

On Thu, Oct 13, 2016 at 01:41:11PM -0700, Vineeth Remanan Pillai wrote:


Yes, the use case is out-of-tree and the code snippet above depicts the use
.
Since kern_path_locked is also not exported, out-of-tree code used kern_path
for the existence check for directories.

One reference about this issue can be seen here.

http://www.gossamer-threads.com/lists/linux/kernel/2459690?do=post_view_flat#2459690

... and in that thread I have asked for details and got no reply whatsoever.
  

We also have a customer who complained about this functionality change.

I understand that there has been no API promises been made to this API. But
since this is an
exported function, the change in function could cause break in out-of-tree
kernel code. I will
rephrase the commit message to say "change in functionality" instead of
regression

In principle, I have no strong objections against exporting kern_path_locked,
provided it really matches what they (whoever they are) need.  I'm still
curious about the context, though - what is that code trying to do?  Depending
on the actual stuff it wants to implement, there might be better primitives
for doing that *and* there might be something worth adding and exporting
that would be a better match.

It's not that kern_path_locked() isn't a sane interface, but... using it
might be a sign of trying to work around something missing in API.  So again,
please post more details.

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.

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

[Al Viro]

On Thu, Oct 13, 2016 at 01:41:11PM -0700, Vineeth Remanan Pillai wrote:

> Yes, the use case is out-of-tree and the code snippet above depicts the use
> .
> Since kern_path_locked is also not exported, out-of-tree code used kern_path
> for the existence check for directories.
> 
> One reference about this issue can be seen here.
> 
> http://www.gossamer-threads.com/lists/linux/kernel/2459690?do=post_view_flat#2459690

... and in that thread I have asked for details and got no reply whatsoever.
 
> We also have a customer who complained about this functionality change.
> 
> I understand that there has been no API promises been made to this API. But
> since this is an
> exported function, the change in function could cause break in out-of-tree
> kernel code. I will
> rephrase the commit message to say "change in functionality" instead of
> regression

In principle, I have no strong objections against exporting kern_path_locked,
provided it really matches what they (whoever they are) need.  I'm still
curious about the context, though - what is that code trying to do?  Depending
on the actual stuff it wants to implement, there might be better primitives
for doing that *and* there might be something worth adding and exporting
that would be a better match.

It's not that kern_path_locked() isn't a sane interface, but... using it
might be a sign of trying to work around something missing in API.  So again,
please post more details.

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

[Al Viro]

On Thu, Oct 13, 2016 at 01:41:11PM -0700, Vineeth Remanan Pillai wrote:

> Yes, the use case is out-of-tree and the code snippet above depicts the use
> .
> Since kern_path_locked is also not exported, out-of-tree code used kern_path
> for the existence check for directories.
> 
> One reference about this issue can be seen here.
> 
> http://www.gossamer-threads.com/lists/linux/kernel/2459690?do=post_view_flat#2459690

... and in that thread I have asked for details and got no reply whatsoever.
 
> We also have a customer who complained about this functionality change.
> 
> I understand that there has been no API promises been made to this API. But
> since this is an
> exported function, the change in function could cause break in out-of-tree
> kernel code. I will
> rephrase the commit message to say "change in functionality" instead of
> regression

In principle, I have no strong objections against exporting kern_path_locked,
provided it really matches what they (whoever they are) need.  I'm still
curious about the context, though - what is that code trying to do?  Depending
on the actual stuff it wants to implement, there might be better primitives
for doing that *and* there might be something worth adding and exporting
that would be a better match.

It's not that kern_path_locked() isn't a sane interface, but... using it
might be a sign of trying to work around something missing in API.  So again,
please post more details.

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

[Al Viro]

On Thu, Oct 13, 2016 at 01:09:04PM -0700, Christoph Hellwig wrote:
> On Thu, Oct 13, 2016 at 07:58:51PM +, Vineeth Remanan Pillai wrote:
> > filename_lookup used to return success for non-existing file when called
> > with LOOKUP_PARENT flag. This behaviour was changed with
> > commit 8bcb77fabd7c ("namei: split off filename_lookupat()
> > with LOOKUP_PARENT")
> > 
> > The above patch split parent lookup functionality to a different function
> > filename_parentat and changed all calls to filename_lookup(LOOKUP_PARENT)
> > to the new function filename_parentat. But functions like kern_path which
> > passed the flags directly to filename_lookup regressed due to this.
> > 
> > This patch aims to fix the regressed behaviour by calling
> > filename_parentat from filename_lookup if the flags contain LOOKUP_PARENT.
> 
> What callers shows te problems?  That's probaby were the fix need to got
> in, and even if not that's still part of a good bug report.

Out-of-tree, at a guess...  Incidentally, since filename_lookup() is not
exported, it's probably kern_path() that gets used wherever it is.
Depending on the details of what's being attempted,  kern_path_locked()
might or might not be the right primitive to use, but I would probably
start with checking if that's what the code in question really wants.

Use:
// kernel_string is an kernel pointer to NUL-terminated array of char

struct path path;
struct dentry *dentry;

dentry = kern_path_locked(kernel_string, );
if (IS_ERR(dentry))
// failed while getting the parent, or not a regular last
// component (/, ., .., /., /..)
sod off // no cleanup needed

// now path contains the resolved parent and dentry points to the
// dentry of child, possibly negative; the last component of the
// name can be determined from dentry->d_name.  Parent directory
// is locked, making sure that directory entry won't be changed
// until you are done.

if (d_is_really_negative(dentry)) {
// parent exists, but child doesn't
} else {
// child exists
}

// clean up: drop dentry, unlock parent, drop dentry/vfsmount of parent
dput(dentry);
inode_unlock(path.dentry->d_inode);
path_put();

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

[Al Viro]

On Thu, Oct 13, 2016 at 01:09:04PM -0700, Christoph Hellwig wrote:
> On Thu, Oct 13, 2016 at 07:58:51PM +, Vineeth Remanan Pillai wrote:
> > filename_lookup used to return success for non-existing file when called
> > with LOOKUP_PARENT flag. This behaviour was changed with
> > commit 8bcb77fabd7c ("namei: split off filename_lookupat()
> > with LOOKUP_PARENT")
> > 
> > The above patch split parent lookup functionality to a different function
> > filename_parentat and changed all calls to filename_lookup(LOOKUP_PARENT)
> > to the new function filename_parentat. But functions like kern_path which
> > passed the flags directly to filename_lookup regressed due to this.
> > 
> > This patch aims to fix the regressed behaviour by calling
> > filename_parentat from filename_lookup if the flags contain LOOKUP_PARENT.
> 
> What callers shows te problems?  That's probaby were the fix need to got
> in, and even if not that's still part of a good bug report.

Out-of-tree, at a guess...  Incidentally, since filename_lookup() is not
exported, it's probably kern_path() that gets used wherever it is.
Depending on the details of what's being attempted,  kern_path_locked()
might or might not be the right primitive to use, but I would probably
start with checking if that's what the code in question really wants.

Use:
// kernel_string is an kernel pointer to NUL-terminated array of char

struct path path;
struct dentry *dentry;

dentry = kern_path_locked(kernel_string, );
if (IS_ERR(dentry))
// failed while getting the parent, or not a regular last
// component (/, ., .., /., /..)
sod off // no cleanup needed

// now path contains the resolved parent and dentry points to the
// dentry of child, possibly negative; the last component of the
// name can be determined from dentry->d_name.  Parent directory
// is locked, making sure that directory entry won't be changed
// until you are done.

if (d_is_really_negative(dentry)) {
// parent exists, but child doesn't
} else {
// child exists
}

// clean up: drop dentry, unlock parent, drop dentry/vfsmount of parent
dput(dentry);
inode_unlock(path.dentry->d_inode);
path_put();

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

[Christoph Hellwig]

On Thu, Oct 13, 2016 at 01:41:11PM -0700, Vineeth Remanan Pillai wrote:
> Yes, the use case is out-of-tree and the code snippet above depicts the use

Case closed then.  If it doesn't affect any in-tree code please don't
bother spamming linux lists with your self-inflicted wounds.

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

[Christoph Hellwig]

On Thu, Oct 13, 2016 at 01:41:11PM -0700, Vineeth Remanan Pillai wrote:
> Yes, the use case is out-of-tree and the code snippet above depicts the use

Case closed then.  If it doesn't affect any in-tree code please don't
bother spamming linux lists with your self-inflicted wounds.

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

[Vineeth Remanan Pillai]

On 10/13/2016 01:26 PM, Al Viro wrote:

On Thu, Oct 13, 2016 at 01:09:04PM -0700, Christoph Hellwig wrote:

On Thu, Oct 13, 2016 at 07:58:51PM +, Vineeth Remanan Pillai wrote:

filename_lookup used to return success for non-existing file when called
with LOOKUP_PARENT flag. This behaviour was changed with
commit 8bcb77fabd7c ("namei: split off filename_lookupat()
with LOOKUP_PARENT")

The above patch split parent lookup functionality to a different function
filename_parentat and changed all calls to filename_lookup(LOOKUP_PARENT)
to the new function filename_parentat. But functions like kern_path which
passed the flags directly to filename_lookup regressed due to this.

This patch aims to fix the regressed behaviour by calling
filename_parentat from filename_lookup if the flags contain LOOKUP_PARENT.

What callers shows te problems?  That's probaby were the fix need to got
in, and even if not that's still part of a good bug report.

Out-of-tree, at a guess...  Incidentally, since filename_lookup() is not
exported, it's probably kern_path() that gets used wherever it is.
Depending on the details of what's being attempted,  kern_path_locked()
might or might not be the right primitive to use, but I would probably
start with checking if that's what the code in question really wants.

Use:
// kernel_string is an kernel pointer to NUL-terminated array of char

struct path path;
struct dentry *dentry;

dentry = kern_path_locked(kernel_string, );
if (IS_ERR(dentry))
// failed while getting the parent, or not a regular last
// component (/, ., .., /., /..)
sod off // no cleanup needed

// now path contains the resolved parent and dentry points to the
// dentry of child, possibly negative; the last component of the
// name can be determined from dentry->d_name.  Parent directory
// is locked, making sure that directory entry won't be changed
// until you are done.

if (d_is_really_negative(dentry)) {
// parent exists, but child doesn't
} else {
// child exists
}

// clean up: drop dentry, unlock parent, drop dentry/vfsmount of parent
dput(dentry);
inode_unlock(path.dentry->d_inode);
path_put();

Yes, the use case is out-of-tree and the code snippet above depicts the 
use .

Since kern_path_locked is also not exported, out-of-tree code used kern_path
for the existence check for directories.

One reference about this issue can be seen here.

http://www.gossamer-threads.com/lists/linux/kernel/2459690?do=post_view_flat#2459690

We also have a customer who complained about this functionality change.

I understand that there has been no API promises been made to this API. 
But since this is an
exported function, the change in function could cause break in 
out-of-tree kernel code. I will
rephrase the commit message to say "change in functionality" instead of 
regression

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

[Vineeth Remanan Pillai]

On 10/13/2016 01:26 PM, Al Viro wrote:

On Thu, Oct 13, 2016 at 01:09:04PM -0700, Christoph Hellwig wrote:

On Thu, Oct 13, 2016 at 07:58:51PM +, Vineeth Remanan Pillai wrote:

filename_lookup used to return success for non-existing file when called
with LOOKUP_PARENT flag. This behaviour was changed with
commit 8bcb77fabd7c ("namei: split off filename_lookupat()
with LOOKUP_PARENT")

The above patch split parent lookup functionality to a different function
filename_parentat and changed all calls to filename_lookup(LOOKUP_PARENT)
to the new function filename_parentat. But functions like kern_path which
passed the flags directly to filename_lookup regressed due to this.

This patch aims to fix the regressed behaviour by calling
filename_parentat from filename_lookup if the flags contain LOOKUP_PARENT.

What callers shows te problems?  That's probaby were the fix need to got
in, and even if not that's still part of a good bug report.

Out-of-tree, at a guess...  Incidentally, since filename_lookup() is not
exported, it's probably kern_path() that gets used wherever it is.
Depending on the details of what's being attempted,  kern_path_locked()
might or might not be the right primitive to use, but I would probably
start with checking if that's what the code in question really wants.

Use:
// kernel_string is an kernel pointer to NUL-terminated array of char

struct path path;
struct dentry *dentry;

dentry = kern_path_locked(kernel_string, );
if (IS_ERR(dentry))
// failed while getting the parent, or not a regular last
// component (/, ., .., /., /..)
sod off // no cleanup needed

// now path contains the resolved parent and dentry points to the
// dentry of child, possibly negative; the last component of the
// name can be determined from dentry->d_name.  Parent directory
// is locked, making sure that directory entry won't be changed
// until you are done.

if (d_is_really_negative(dentry)) {
// parent exists, but child doesn't
} else {
// child exists
}

// clean up: drop dentry, unlock parent, drop dentry/vfsmount of parent
dput(dentry);
inode_unlock(path.dentry->d_inode);
path_put();

Yes, the use case is out-of-tree and the code snippet above depicts the 
use .

Since kern_path_locked is also not exported, out-of-tree code used kern_path
for the existence check for directories.

One reference about this issue can be seen here.

http://www.gossamer-threads.com/lists/linux/kernel/2459690?do=post_view_flat#2459690

We also have a customer who complained about this functionality change.

I understand that there has been no API promises been made to this API. 
But since this is an
exported function, the change in function could cause break in 
out-of-tree kernel code. I will
rephrase the commit message to say "change in functionality" instead of 
regression

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

[Christoph Hellwig]

On Thu, Oct 13, 2016 at 07:58:51PM +, Vineeth Remanan Pillai wrote:
> filename_lookup used to return success for non-existing file when called
> with LOOKUP_PARENT flag. This behaviour was changed with
> commit 8bcb77fabd7c ("namei: split off filename_lookupat()
> with LOOKUP_PARENT")
> 
> The above patch split parent lookup functionality to a different function
> filename_parentat and changed all calls to filename_lookup(LOOKUP_PARENT)
> to the new function filename_parentat. But functions like kern_path which
> passed the flags directly to filename_lookup regressed due to this.
> 
> This patch aims to fix the regressed behaviour by calling
> filename_parentat from filename_lookup if the flags contain LOOKUP_PARENT.

What callers shows te problems?  That's probaby were the fix need to got
in, and even if not that's still part of a good bug report.

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

[Christoph Hellwig]

On Thu, Oct 13, 2016 at 07:58:51PM +, Vineeth Remanan Pillai wrote:
> filename_lookup used to return success for non-existing file when called
> with LOOKUP_PARENT flag. This behaviour was changed with
> commit 8bcb77fabd7c ("namei: split off filename_lookupat()
> with LOOKUP_PARENT")
> 
> The above patch split parent lookup functionality to a different function
> filename_parentat and changed all calls to filename_lookup(LOOKUP_PARENT)
> to the new function filename_parentat. But functions like kern_path which
> passed the flags directly to filename_lookup regressed due to this.
> 
> This patch aims to fix the regressed behaviour by calling
> filename_parentat from filename_lookup if the flags contain LOOKUP_PARENT.

What callers shows te problems?  That's probaby were the fix need to got
in, and even if not that's still part of a good bug report.

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

[Al Viro]

On Thu, Oct 13, 2016 at 07:58:51PM +, Vineeth Remanan Pillai wrote:
> filename_lookup used to return success for non-existing file when called
> with LOOKUP_PARENT flag. This behaviour was changed with
> commit 8bcb77fabd7c ("namei: split off filename_lookupat()
> with LOOKUP_PARENT")
> 
> The above patch split parent lookup functionality to a different function
> filename_parentat and changed all calls to filename_lookup(LOOKUP_PARENT)
> to the new function filename_parentat. But functions like kern_path which
> passed the flags directly to filename_lookup regressed due to this.
> 
> This patch aims to fix the regressed behaviour by calling
> filename_parentat from filename_lookup if the flags contain LOOKUP_PARENT.

What would we want that for?  Details, please.  In particular, I would like
to know how to use that in non-racy way.  What are you doing with it?

PS: "regressed" assumes that there had been any promise of API stability;
no such thing has ever existed.

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

[Al Viro]

On Thu, Oct 13, 2016 at 07:58:51PM +, Vineeth Remanan Pillai wrote:
> filename_lookup used to return success for non-existing file when called
> with LOOKUP_PARENT flag. This behaviour was changed with
> commit 8bcb77fabd7c ("namei: split off filename_lookupat()
> with LOOKUP_PARENT")
> 
> The above patch split parent lookup functionality to a different function
> filename_parentat and changed all calls to filename_lookup(LOOKUP_PARENT)
> to the new function filename_parentat. But functions like kern_path which
> passed the flags directly to filename_lookup regressed due to this.
> 
> This patch aims to fix the regressed behaviour by calling
> filename_parentat from filename_lookup if the flags contain LOOKUP_PARENT.

What would we want that for?  Details, please.  In particular, I would like
to know how to use that in non-racy way.  What are you doing with it?

PS: "regressed" assumes that there had been any promise of API stability;
no such thing has ever existed.