| 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
| ||
|
Message-id: <1329993854.30585.3.camel@offworld>
Date: Thu, 23 Feb 2012 11:44:14 +0100
From: Davidlohr Bueso <dave@....org>
To: "Eric W. Biederman" <ebiederm@...ssion.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
"J. Bruce Fields" <bfields@...ldses.org>,
Matthew Wilcox <matthew@....cx>,
lkml <linux-kernel@...r.kernel.org>,
linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] locks: new procfs lockinfo
On Wed, 2012-02-22 at 16:38 -0800, Eric W. Biederman wrote:
> Davidlohr Bueso <dave@....org> writes:
>
> > From: Davidlohr Bueso <dave@....org>
> >
> > Based on our previous discussion https://lkml.org/lkml/2012/2/10/462 we came to
> > agree on deprecating the current /proc/locks in favor of a more extensible interface.
> > The new /proc/lockinfo file exports similar information - except instead of maj:min the
> > device name is shown - and entries are formated like those in /proc/cpuinfo, allowing us
> > to add new entries without breaking userspace.
>
> You can't know the device name, attempt to say what you don't know seems
> very dangerous. It may be reasonable to simply give the deivce number
> and not split the device number into major/minor any more and I am
> concerned about reality.
>
> Andrew's question about the pid namespace is answered below.
>
> > Signed-off-by: Davidlohr Bueso <dave@....org>
> > ---
> > Documentation/feature-removal-schedule.txt | 9 +++
> > fs/locks.c | 109 ++++++++++++++++++++++++++--
> > 2 files changed, 113 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
> > index a0ffac0..1c5e14b 100644
> > --- a/Documentation/feature-removal-schedule.txt
> > +++ b/Documentation/feature-removal-schedule.txt
> > @@ -524,3 +524,12 @@ Files: arch/arm/mach-at91/at91cap9.c
> > Why: The code is not actively maintained and platforms are now hard to find.
> > Who: Nicolas Ferre <nicolas.ferre@...el.com>
> > Jean-Christophe PLAGNIOL-VILLARD <plagnioj@...osoft.com>
> > +
> > +---------------------------
> > +
> > +What: /proc/locks
> > +When: 2014
> > +Why: The current /proc/locks file does not allow modifying entries as it breaks
> > + userspace (most notably lslk(8)). A new /proc/lockinfo interface replaces
> > + this file in a more extendable format (lines per entry), like /proc/cpuinfo.
> > +Who: Davidlohr Bueso <dave@....org>
>
> This is a ancient file of long standing I really doubt that we can
> safely remove it any time soon. Is there any good reason to want to
> remove this file?
>
> > diff --git a/fs/locks.c b/fs/locks.c
> > index 637694b..f7b27fe 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -112,6 +112,9 @@
> > * Leases and LOCK_MAND
> > * Matthew Wilcox <willy@...ian.org>, June, 2000.
> > * Stephen Rothwell <sfr@...b.auug.org.au>, June, 2000.
> > + *
> > + * Deprecated /proc/locks in favor of /proc/lockinfo
> > + * Davidlohr Bueso <dave@....org>, February, 2012.
> > */
> >
> > #include <linux/capability.h>
> > @@ -2156,6 +2159,10 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
> > struct inode *inode = NULL;
> > unsigned int fl_pid;
> >
> > + /* deprecated, see Documentation/feature-removal-schedule.txt */
> > + printk_once(KERN_WARNING "%s (%d): /proc/locks is deprecated please use /proc/lockinfo instead.\n",
> > + current->comm, task_pid_nr(current));
> > +
> > if (fl->fl_nspid)
> > fl_pid = pid_vnr(fl->fl_nspid);
>
> Apparently this was overlooked. Sigh.
>
> We need not to use pid_vnr but instead pid_nr_ns(sb->s_fs_info, fl->fl_nspid);
>
> For using this outside of fs/proc/base.c this clearly needs a trivial
> helper instead of raw s_fs_info access, but the point remains that
> the proc filesystem when mounted has a pid namespace that it displays
> everything relative too and /proc/locks should be the same.
>
> > else
> > @@ -2199,15 +2206,10 @@ static void lock_get_status(struct seq_file *f, struct file_lock *fl,
> > : (fl->fl_type & F_WRLCK) ? "WRITE" : "READ ");
> > }
> > if (inode) {
> > -#ifdef WE_CAN_BREAK_LSLK_NOW
> > - seq_printf(f, "%d %s:%ld ", fl_pid,
> > - inode->i_sb->s_id, inode->i_ino);
> > -#else
> > /* userspace relies on this representation of dev_t ;-( */
> > seq_printf(f, "%d %02x:%02x:%ld ", fl_pid,
> > MAJOR(inode->i_sb->s_dev),
> > MINOR(inode->i_sb->s_dev), inode->i_ino);
> > -#endif
> > } else {
> > seq_printf(f, "%d <none>:0 ", fl_pid);
> > }
> > @@ -2275,9 +2277,106 @@ static const struct file_operations proc_locks_operations = {
> > .release = seq_release_private,
> > };
> >
> > +static void lockinfo_get_status(struct seq_file *f, struct file_lock *fl,
> > + loff_t id)
> > +{
> > + struct inode *inode = NULL;
> > + unsigned int fl_pid;
> > +
> > + if (fl->fl_nspid)
> > + fl_pid = pid_vnr(fl->fl_nspid);
>
> We shouldn't copy the wrong definition for fl_pid from the old code but
> should instead get this right.
Will correct.
>
> > + else
> > + fl_pid = fl->fl_pid;
> > +
> > + if (fl->fl_file != NULL)
> > + inode = fl->fl_file->f_path.dentry->d_inode;
> > +
> > + if (IS_POSIX(fl)) {
> > + seq_printf(f, "Personality:\t %s\n",
> > + (fl->fl_flags & FL_ACCESS) ? "ACCESS" : "POSIX ");
> > + seq_printf(f, "Type:\t\t %s\n",
> > + (!inode) ? "*NOINODE*" : mandatory_lock(inode)
> > + ? "MANDATORY" : "ADVISORY ");
> > + } else if (IS_FLOCK(fl)) {
> > + seq_printf(f, "Personality:\t FLOCK\n");
> > + seq_printf(f, "Type:\t\t %s\n",
> > + (fl->fl_type & LOCK_MAND) ? "MSNFS" : "ADVISORY");
> > + } else if (IS_LEASE(fl)) {
> > + seq_printf(f, "Personality:\t LEASE\n");
> > + seq_printf(f, "Type:\t\t %s\n",
> > + (lease_breaking(fl)) ? "BREAKING"
> > + : (fl->fl_file) ? "ACTIVE" : "BREAKER");
> > + } else {
> > + seq_printf(f, "Personality:\t UNKNOWN\n");
> > + seq_printf(f, "Type:\t\t UNKNOWN\n");
> > + }
> > +
> > + if (fl->fl_type & LOCK_MAND) {
> > + seq_printf(f, "Access:\t\t %s\n",
> > + (fl->fl_type & LOCK_READ)
> > + ? (fl->fl_type & LOCK_WRITE) ? "RW " : "READ "
> > + : (fl->fl_type & LOCK_WRITE) ? "WRITE" : "NONE ");
> > + } else {
> > + seq_printf(f, "Access:\t\t %s\n",
> > + (lease_breaking(fl))
> > + ? (fl->fl_type & F_UNLCK) ? "UNLCK" : "READ "
> > + : (fl->fl_type & F_WRLCK) ? "WRITE" : "READ ");
> > + }
> > +
> > + seq_printf(f, "PID:\t\t %d\n", fl_pid);
> > +
> > + if (inode) {
> > + seq_printf(f, "Device:\t\t %s\n", inode->i_sb->s_id);
>
> Hmm. The label on this is wrong. What if this comes from a
> filesystem that is not block device based? I expect it is ok to print
> sb->s_id here but it needs a less confusing label.
I think that even if its not a block based device, the "Device" label is
pretty obvious.
I'll send a v2 of this patch with the corrections and documenting the
new file in Documentation/filesystems/proc.txt
Thanks,
Davidlohr
--
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/
Powered by blists - more mailing lists