| 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: <alpine.LSU.2.00.1105120929190.2735@sister.anvils>
Date: Thu, 12 May 2011 09:52:04 -0700 (PDT)
From: Hugh Dickins <hughd@...gle.com>
To: Miklos Szeredi <miklos@...redi.hu>
cc: Michal Suchanek <hramrach@...trum.cz>,
Andreas Dilger <adilger@...ger.ca>,
Jiri Kosina <jkosina@...e.cz>,
Ric Wheeler <ricwheeler@...il.com>,
linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
David Howells <dhowells@...hat.com>,
Ian Kent <ikent@...hat.com>, Jeff Moyer <jmoyer@...hat.com>,
Christoph Hellwig <hch@...radead.org>,
Eric Paris <eparis@...hat.com>,
Andrew Morton <akpm@...ux-foundation.org>,
James Morris <jmorris@...ei.org>,
Serge Hallyn <serge.hallyn@...ntu.com>
Subject: Re: [PATCH] tmpfs: implement generic xattr support
On Thu, 12 May 2011, Miklos Szeredi wrote:
> Miklos Szeredi <miklos@...redi.hu> writes:
>
> >>> + info = SHMEM_I(dentry->d_inode);
> >>> +
> >>> + spin_lock(&dentry->d_inode->i_lock);
> >>
> >> Not important, but I suggest you use info->lock throughout for this,
> >> instead of dentry->d_inode->i_lock: in each case you need "info" for
> >> info->xattr_list (and don't need "inode" at all I think), so info->lock
> >> seems appropriate, and may be in the same cacheline once I make
> >> shmem_inode_info smaller. But don't worry if you'd prefer to leave
> >> it.
> >
> > Makes sense. I updated the locking.
>
> This uncovered a nasty bug lurking in there: the "info" area, including
> lock and xattr_list, may be overwritten by inline symlinks. Because
> xattr_list is near the end, this wasn't noticed with casual testing, but
> info->lock would immediately Oops on getxattr for symlinks.
Yikes, I'd completely forgotten about those inline symlinks:
many thanks for reminding me.
>
> I propose the following solution. It results in slightly less space for
> inline symlinks, but correct operation for xattrs. Does the anonymous
> union/struct solution look acceptable?
You're being conscientious to minimize the space reduction, and I wonder
if I'm being sloppy about it: but I think I'd prefer you to keep it simple
and just make a union of the i_direct[SHMEM_NR_DIRECT] array and the inline
symlink buffer. That does waste space that was occasionally being put to
use before, but saves us from embarrassment next time we forget about the
inline symlinks.
I intend to be removing that i_direct array very soon: I guess I'll want
to kmalloc for short symlinks then, certainly not overlaying over what
fields are left: so you'd be moving in that direction if you just reuse
the i_direct area now.
Hugh
>
> Thanks,
> Miklos
>
> Index: linux-2.6/include/linux/shmem_fs.h
> ===================================================================
> --- linux-2.6.orig/include/linux/shmem_fs.h 2011-05-12 15:59:08.000000000 +0200
> +++ linux-2.6/include/linux/shmem_fs.h 2011-05-12 15:58:25.000000000 +0200
> @@ -11,15 +11,39 @@
>
> struct shmem_inode_info {
> spinlock_t lock;
> - unsigned long flags;
> - unsigned long alloced; /* data pages alloced to file */
> - unsigned long swapped; /* subtotal assigned to swap */
> - unsigned long next_index; /* highest alloced index + 1 */
> - struct shared_policy policy; /* NUMA memory alloc policy */
> - struct page *i_indirect; /* top indirect blocks page */
> - swp_entry_t i_direct[SHMEM_NR_DIRECT]; /* first blocks */
> - struct list_head swaplist; /* chain of maybes on swap */
> - struct list_head xattr_list; /* list of shmem_xattr */
> +
> + /* list of shmem_xattr */
> + struct list_head xattr_list;
> +
> + union {
> + char inline_symlink[0];
> +
> + /* Members not used by inline symlinks: */
> + struct {
> + unsigned long flags;
> +
> + /* data pages alloced to file */
> + unsigned long alloced;
> +
> + /* subtotal assigned to swap */
> + unsigned long swapped;
> +
> + /* highest alloced index + 1 */
> + unsigned long next_index;
> +
> + /* NUMA memory alloc policy */
> + struct shared_policy policy;
> +
> + /* top indirect blocks page */
> + struct page *i_indirect;
> +
> + /* first blocks */
> + swp_entry_t i_direct[SHMEM_NR_DIRECT];
> +
> + /* chain of maybes on swap */
> + struct list_head swaplist;
> + };
> + };
> struct inode vfs_inode;
> };
>
> Index: linux-2.6/mm/shmem.c
> ===================================================================
> --- linux-2.6.orig/mm/shmem.c 2011-05-12 15:59:08.000000000 +0200
> +++ linux-2.6/mm/shmem.c 2011-05-12 15:50:10.000000000 +0200
> @@ -2029,9 +2029,9 @@ static int shmem_symlink(struct inode *d
>
> info = SHMEM_I(inode);
> inode->i_size = len-1;
> - if (len <= (char *)inode - (char *)info) {
> + if (len <= (char *)inode - info->inline_symlink) {
> /* do it inline */
> - memcpy(info, symname, len);
> + memcpy(info->inline_symlink, symname, len);
> inode->i_op = &shmem_symlink_inline_operations;
> } else {
> error = shmem_getpage(inode, 0, &page, SGP_WRITE, NULL);
> @@ -2057,7 +2057,7 @@ static int shmem_symlink(struct inode *d
>
> static void *shmem_follow_link_inline(struct dentry *dentry, struct nameidata *nd)
> {
> - nd_set_link(nd, (char *)SHMEM_I(dentry->d_inode));
> + nd_set_link(nd, SHMEM_I(dentry->d_inode)->inline_symlink);
> return NULL;
> }
--
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