* [PATCH] proc: Fix namespace mountpoint path in /proc/mounts @ 2013-10-25 21:51 Aditya Kali 2013-10-29 15:36 ` Serge E. Hallyn 0 siblings, 1 reply; 6+ messages in thread From: Aditya Kali @ 2013-10-25 21:51 UTC (permalink / raw) To: ebiederm, viro, linux-fsdevel, linux-kernel; +Cc: Aditya Kali Commit bf056bfa80596a5d14b26b17276a56a0dcb080e5: "proc: Fix the namespace inode permission checks." converted the namespace files into symlinks. The same commit changed the way namespace bind mounts appear in /proc/mounts: $ mount --bind /proc/self/ns/ipc /mnt/ipc Originally: $ cat /proc/mounts | grep ipc proc /mnt/ipc proc rw,nosuid,nodev,noexec 0 0 After commit bf056bfa80596a5d14b26b17276a56a0dcb080e5: $ cat /proc/mounts | grep ipc proc ipc:[4026531839] proc rw,nosuid,nodev,noexec 0 0 This breaks userspace which expects the 2nd field in /proc/mounts to be a valid path. This patch restores the original format of namespace bind-mount entries in /proc/mounts. Signed-off-by: Aditya Kali <adityakali@google.com> --- fs/proc/namespaces.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c index 49a7fff..d19989d 100644 --- a/fs/proc/namespaces.c +++ b/fs/proc/namespaces.c @@ -48,19 +48,9 @@ static int ns_delete_dentry(const struct dentry *dentry) return 1; } -static char *ns_dname(struct dentry *dentry, char *buffer, int buflen) -{ - struct inode *inode = dentry->d_inode; - const struct proc_ns_operations *ns_ops = PROC_I(inode)->ns.ns_ops; - - return dynamic_dname(dentry, buffer, buflen, "%s:[%lu]", - ns_ops->name, inode->i_ino); -} - const struct dentry_operations ns_dentry_operations = { .d_delete = ns_delete_dentry, - .d_dname = ns_dname, }; static struct dentry *proc_ns_get_dentry(struct super_block *sb, -- 1.8.4.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] proc: Fix namespace mountpoint path in /proc/mounts 2013-10-25 21:51 [PATCH] proc: Fix namespace mountpoint path in /proc/mounts Aditya Kali @ 2013-10-29 15:36 ` Serge E. Hallyn 2013-11-08 23:54 ` Eric W. Biederman 0 siblings, 1 reply; 6+ messages in thread From: Serge E. Hallyn @ 2013-10-29 15:36 UTC (permalink / raw) To: Aditya Kali; +Cc: ebiederm, viro, linux-fsdevel, linux-kernel Quoting Aditya Kali (adityakali@google.com): > Commit bf056bfa80596a5d14b26b17276a56a0dcb080e5: > "proc: Fix the namespace inode permission checks." converted > the namespace files into symlinks. The same commit changed > the way namespace bind mounts appear in /proc/mounts: > $ mount --bind /proc/self/ns/ipc /mnt/ipc > Originally: > $ cat /proc/mounts | grep ipc > proc /mnt/ipc proc rw,nosuid,nodev,noexec 0 0 > > After commit bf056bfa80596a5d14b26b17276a56a0dcb080e5: > $ cat /proc/mounts | grep ipc > proc ipc:[4026531839] proc rw,nosuid,nodev,noexec 0 0 > > This breaks userspace which expects the 2nd field in > /proc/mounts to be a valid path. This patch restores the > original format of namespace bind-mount entries in > /proc/mounts. Oh, at first I was thinking the mount source was showing up like that, not the target. That is particularly ugly, I agree. I'm not sure what the purpose was of the ns_dname(). dcache.c says it's for filesystems wanting to do 'special "root names"'. But this file gets mounted to real paths, it's not actually rootless (like a pipefs inode or anon_inode). So I think your patch is correct, but I'm waiting to hear from Eric, as I'm not sure if you're masking some other effect which Eric actually wanted, and maybe this should be fixed another way... > Signed-off-by: Aditya Kali <adityakali@google.com> > --- > fs/proc/namespaces.c | 10 ---------- > 1 file changed, 10 deletions(-) > > diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c > index 49a7fff..d19989d 100644 > --- a/fs/proc/namespaces.c > +++ b/fs/proc/namespaces.c > @@ -48,19 +48,9 @@ static int ns_delete_dentry(const struct dentry *dentry) > return 1; > } > > -static char *ns_dname(struct dentry *dentry, char *buffer, int buflen) > -{ > - struct inode *inode = dentry->d_inode; > - const struct proc_ns_operations *ns_ops = PROC_I(inode)->ns.ns_ops; > - > - return dynamic_dname(dentry, buffer, buflen, "%s:[%lu]", > - ns_ops->name, inode->i_ino); > -} > - > const struct dentry_operations ns_dentry_operations = > { > .d_delete = ns_delete_dentry, > - .d_dname = ns_dname, > }; > > static struct dentry *proc_ns_get_dentry(struct super_block *sb, > -- > 1.8.4.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] proc: Fix namespace mountpoint path in /proc/mounts 2013-10-29 15:36 ` Serge E. Hallyn @ 2013-11-08 23:54 ` Eric W. Biederman 2013-11-21 21:50 ` Serge E. Hallyn 0 siblings, 1 reply; 6+ messages in thread From: Eric W. Biederman @ 2013-11-08 23:54 UTC (permalink / raw) To: Serge E. Hallyn; +Cc: Aditya Kali, viro, linux-fsdevel, linux-kernel "Serge E. Hallyn" <serge@hallyn.com> writes: > Quoting Aditya Kali (adityakali@google.com): >> Commit bf056bfa80596a5d14b26b17276a56a0dcb080e5: >> "proc: Fix the namespace inode permission checks." converted >> the namespace files into symlinks. The same commit changed >> the way namespace bind mounts appear in /proc/mounts: >> $ mount --bind /proc/self/ns/ipc /mnt/ipc >> Originally: >> $ cat /proc/mounts | grep ipc >> proc /mnt/ipc proc rw,nosuid,nodev,noexec 0 0 >> >> After commit bf056bfa80596a5d14b26b17276a56a0dcb080e5: >> $ cat /proc/mounts | grep ipc >> proc ipc:[4026531839] proc rw,nosuid,nodev,noexec 0 0 >> >> This breaks userspace which expects the 2nd field in >> /proc/mounts to be a valid path. This patch restores the >> original format of namespace bind-mount entries in >> /proc/mounts. > > Oh, at first I was thinking the mount source was showing up like that, > not the target. That is particularly ugly, I agree. > > I'm not sure what the purpose was of the ns_dname(). dcache.c says it's > for filesystems wanting to do 'special "root names"'. But this file > gets mounted to real paths, it's not actually rootless (like a pipefs > inode or anon_inode). So I think your patch is correct, but I'm waiting > to hear from Eric, as I'm not sure if you're masking some other effect > which Eric actually wanted, and maybe this should be fixed another > way... My apologies for taking a long time to get back to this one. I have been scratching my head on this one. There is most definitely a bug here, and worth fixing. But I believe the bug is actually in buried in /proc/mounts. ns_dname should be irrelevant as we are mounted. The problem comes down to d_path. I am not certain which is the best fix at the moment. It should either be a case of fixing d_path to see if the dentry is mounted, or making certain that the dentries have the name ns_dname is giving them when we allocate the dentries. I was focusing on the what a ns file descriptor should look like when it is opened but not mounted, when I wrote ns_dname, and that appearance really should continue if possible. I expect the easist way to fix this is to simply modify proc_ns_get_dentry to compute the dentry name that ns_dname uses today, and pass that name to d_alloc_psuedo. At which point we can delete ns_dname without problems. Eric >> Signed-off-by: Aditya Kali <adityakali@google.com> >> --- >> fs/proc/namespaces.c | 10 ---------- >> 1 file changed, 10 deletions(-) >> >> diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c >> index 49a7fff..d19989d 100644 >> --- a/fs/proc/namespaces.c >> +++ b/fs/proc/namespaces.c >> @@ -48,19 +48,9 @@ static int ns_delete_dentry(const struct dentry *dentry) >> return 1; >> } >> >> -static char *ns_dname(struct dentry *dentry, char *buffer, int buflen) >> -{ >> - struct inode *inode = dentry->d_inode; >> - const struct proc_ns_operations *ns_ops = PROC_I(inode)->ns.ns_ops; >> - >> - return dynamic_dname(dentry, buffer, buflen, "%s:[%lu]", >> - ns_ops->name, inode->i_ino); >> -} >> - >> const struct dentry_operations ns_dentry_operations = >> { >> .d_delete = ns_delete_dentry, >> - .d_dname = ns_dname, >> }; >> >> static struct dentry *proc_ns_get_dentry(struct super_block *sb, >> -- >> 1.8.4.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] proc: Fix namespace mountpoint path in /proc/mounts 2013-11-08 23:54 ` Eric W. Biederman @ 2013-11-21 21:50 ` Serge E. Hallyn 2013-11-21 22:46 ` Eric W. Biederman 0 siblings, 1 reply; 6+ messages in thread From: Serge E. Hallyn @ 2013-11-21 21:50 UTC (permalink / raw) To: Eric W. Biederman Cc: Serge E. Hallyn, Aditya Kali, viro, linux-fsdevel, linux-kernel Quoting Eric W. Biederman (ebiederm@xmission.com): > "Serge E. Hallyn" <serge@hallyn.com> writes: > > > Quoting Aditya Kali (adityakali@google.com): > >> Commit bf056bfa80596a5d14b26b17276a56a0dcb080e5: > >> "proc: Fix the namespace inode permission checks." converted > >> the namespace files into symlinks. The same commit changed > >> the way namespace bind mounts appear in /proc/mounts: > >> $ mount --bind /proc/self/ns/ipc /mnt/ipc > >> Originally: > >> $ cat /proc/mounts | grep ipc > >> proc /mnt/ipc proc rw,nosuid,nodev,noexec 0 0 > >> > >> After commit bf056bfa80596a5d14b26b17276a56a0dcb080e5: > >> $ cat /proc/mounts | grep ipc > >> proc ipc:[4026531839] proc rw,nosuid,nodev,noexec 0 0 > >> > >> This breaks userspace which expects the 2nd field in > >> /proc/mounts to be a valid path. This patch restores the > >> original format of namespace bind-mount entries in > >> /proc/mounts. > > > > Oh, at first I was thinking the mount source was showing up like that, > > not the target. That is particularly ugly, I agree. > > > > I'm not sure what the purpose was of the ns_dname(). dcache.c says it's > > for filesystems wanting to do 'special "root names"'. But this file > > gets mounted to real paths, it's not actually rootless (like a pipefs > > inode or anon_inode). So I think your patch is correct, but I'm waiting > > to hear from Eric, as I'm not sure if you're masking some other effect > > which Eric actually wanted, and maybe this should be fixed another > > way... > > My apologies for taking a long time to get back to this one. I have > been scratching my head on this one. > > There is most definitely a bug here, and worth fixing. > > But I believe the bug is actually in buried in /proc/mounts. ns_dname > should be irrelevant as we are mounted. > > The problem comes down to d_path. > > I am not certain which is the best fix at the moment. It should either > be a case of fixing d_path to see if the dentry is mounted, or making > certain that the dentries have the name ns_dname is giving them when > we allocate the dentries. > > I was focusing on the what a ns file descriptor should look like when it > is opened but not mounted, when I wrote ns_dname, and that appearance > really should continue if possible. > > I expect the easist way to fix this is to simply modify proc_ns_get_dentry to > compute the dentry name that ns_dname uses today, and pass that name to > d_alloc_psuedo. > > At which point we can delete ns_dname without problems. Aditya, is this something you'd have time to write a patch for? > Eric > > > >> Signed-off-by: Aditya Kali <adityakali@google.com> > >> --- > >> fs/proc/namespaces.c | 10 ---------- > >> 1 file changed, 10 deletions(-) > >> > >> diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c > >> index 49a7fff..d19989d 100644 > >> --- a/fs/proc/namespaces.c > >> +++ b/fs/proc/namespaces.c > >> @@ -48,19 +48,9 @@ static int ns_delete_dentry(const struct dentry *dentry) > >> return 1; > >> } > >> > >> -static char *ns_dname(struct dentry *dentry, char *buffer, int buflen) > >> -{ > >> - struct inode *inode = dentry->d_inode; > >> - const struct proc_ns_operations *ns_ops = PROC_I(inode)->ns.ns_ops; > >> - > >> - return dynamic_dname(dentry, buffer, buflen, "%s:[%lu]", > >> - ns_ops->name, inode->i_ino); > >> -} > >> - > >> const struct dentry_operations ns_dentry_operations = > >> { > >> .d_delete = ns_delete_dentry, > >> - .d_dname = ns_dname, > >> }; > >> > >> static struct dentry *proc_ns_get_dentry(struct super_block *sb, > >> -- > >> 1.8.4.1 > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >> Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] proc: Fix namespace mountpoint path in /proc/mounts 2013-11-21 21:50 ` Serge E. Hallyn @ 2013-11-21 22:46 ` Eric W. Biederman 2013-11-21 22:52 ` Serge E. Hallyn 0 siblings, 1 reply; 6+ messages in thread From: Eric W. Biederman @ 2013-11-21 22:46 UTC (permalink / raw) To: Serge E. Hallyn; +Cc: Aditya Kali, viro, linux-fsdevel, linux-kernel "Serge E. Hallyn" <serge@hallyn.com> writes: > Aditya, is this something you'd have time to write a patch for? I have a tested patch that fixes when we call ns_dname. Give me a day and I will push out my pending pile of fixes for review. Eric ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] proc: Fix namespace mountpoint path in /proc/mounts 2013-11-21 22:46 ` Eric W. Biederman @ 2013-11-21 22:52 ` Serge E. Hallyn 0 siblings, 0 replies; 6+ messages in thread From: Serge E. Hallyn @ 2013-11-21 22:52 UTC (permalink / raw) To: Eric W. Biederman Cc: Serge E. Hallyn, Aditya Kali, viro, linux-fsdevel, linux-kernel Quoting Eric W. Biederman (ebiederm@xmission.com): > "Serge E. Hallyn" <serge@hallyn.com> writes: > > > Aditya, is this something you'd have time to write a patch for? > > I have a tested patch that fixes when we call ns_dname. Give me a day > and I will push out my pending pile of fixes for review. > > Eric Oh, awesome, thanks! -serge ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-11-21 22:52 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-10-25 21:51 [PATCH] proc: Fix namespace mountpoint path in /proc/mounts Aditya Kali 2013-10-29 15:36 ` Serge E. Hallyn 2013-11-08 23:54 ` Eric W. Biederman 2013-11-21 21:50 ` Serge E. Hallyn 2013-11-21 22:46 ` Eric W. Biederman 2013-11-21 22:52 ` Serge E. Hallyn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).