[PATCH] tmpfs 1/7 LTP ENAMETOOLONG

[PATCH] tmpfs 1/7 LTP ENAMETOOLONG

[Hugh Dickins]

First of 2+3+2 tmpfs patches, based on 2.6.0-test7-mm1, total diffstat:

 fs/hugetlbfs/inode.c |   20 +++++++++++++---
 fs/libfs.c           |    2 +
 fs/ramfs/inode.c     |    7 +++++
 mm/shmem.c           |   63 +++++++++++++++++++++++++++++++++++++++++----------
 4 files changed, 77 insertions(+), 15 deletions(-)

[PATCH] tmpfs 1/7 LTP ENAMETOOLONG

LTP tests the filesystem on /tmp: many failures when tmpfs because
simple_lookup forgot to reject filenames longer than the NAME_MAX
tmpfs declares in its statfs.  This also fixes ramfs and hugetlbfs.

--- 2.6.0-test7-mm1/fs/libfs.c	2003-10-08 20:24:55.000000000 +0100
+++ tmpfs1/fs/libfs.c	2003-10-15 15:38:30.349562896 +0100
@@ -32,6 +32,8 @@
 
 struct dentry *simple_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
 {
+	if (dentry->d_name.len > NAME_MAX)
+		return ERR_PTR(-ENAMETOOLONG);
 	d_add(dentry, NULL);
 	return NULL;
 }

[PATCH] tmpfs 2/7 LTP S_ISGID dir

[Hugh Dickins]

LTP tests the filesystem on /tmp: many failures when tmpfs because
it missed the way giddy directories hand down their gid.  Also fix
ramfs and hugetlbfs.

--- tmpfs1/fs/hugetlbfs/inode.c	2003-10-15 08:49:54.000000000 +0100
+++ tmpfs2/fs/hugetlbfs/inode.c	2003-10-15 15:38:41.333893024 +0100
@@ -409,10 +409,18 @@
 static int hugetlbfs_mknod(struct inode *dir,
 			struct dentry *dentry, int mode, dev_t dev)
 {
-	struct inode *inode = hugetlbfs_get_inode(dir->i_sb, current->fsuid, 
-					current->fsgid, mode, dev);
+	struct inode *inode;
 	int error = -ENOSPC;
+	gid_t gid;
 
+	if (dir->i_mode & S_ISGID) {
+		gid = dir->i_gid;
+		if (S_ISDIR(mode))
+			mode |= S_ISGID;
+	} else {
+		gid = current->fsgid;
+	}
+	inode = hugetlbfs_get_inode(dir->i_sb, current->fsuid, gid, mode, dev);
 	if (inode) {
 		dir->i_size += PSEUDO_DIRENT_SIZE;
 		dir->i_ctime = dir->i_mtime = CURRENT_TIME;
@@ -441,9 +449,15 @@
 {
 	struct inode *inode;
 	int error = -ENOSPC;
+	gid_t gid;
+
+	if (dir->i_mode & S_ISGID)
+		gid = dir->i_gid;
+	else
+		gid = current->fsgid;
 
 	inode = hugetlbfs_get_inode(dir->i_sb, current->fsuid,
-					current->fsgid, S_IFLNK|S_IRWXUGO, 0);
+					gid, S_IFLNK|S_IRWXUGO, 0);
 	if (inode) {
 		int l = strlen(symname)+1;
 		error = page_symlink(inode, symname, l);
--- tmpfs1/fs/ramfs/inode.c	2003-09-08 20:51:04.000000000 +0100
+++ tmpfs2/fs/ramfs/inode.c	2003-10-15 15:38:41.333893024 +0100
@@ -95,6 +95,11 @@
 	int error = -ENOSPC;
 
 	if (inode) {
+		if (dir->i_mode & S_ISGID) {
+			inode->i_gid = dir->i_gid;
+			if (S_ISDIR(mode))
+				inode->i_mode |= S_ISGID;
+		}
 		d_instantiate(dentry, inode);
 		dget(dentry);	/* Extra count - pin the dentry in core */
 		error = 0;
@@ -125,6 +130,8 @@
 		int l = strlen(symname)+1;
 		error = page_symlink(inode, symname, l);
 		if (!error) {
+			if (dir->i_mode & S_ISGID)
+				inode->i_gid = dir->i_gid;
 			d_instantiate(dentry, inode);
 			dget(dentry);
 		} else
--- tmpfs1/mm/shmem.c	2003-10-15 08:47:31.000000000 +0100
+++ tmpfs2/mm/shmem.c	2003-10-15 15:38:41.336892568 +0100
@@ -1395,6 +1395,11 @@
 	int error = -ENOSPC;
 
 	if (inode) {
+		if (dir->i_mode & S_ISGID) {
+			inode->i_gid = dir->i_gid;
+			if (S_ISDIR(mode))
+				inode->i_mode |= S_ISGID;
+		}
 		dir->i_size += BOGO_DIRENT_SIZE;
 		dir->i_ctime = dir->i_mtime = CURRENT_TIME;
 		d_instantiate(dentry, inode);
@@ -1531,6 +1536,8 @@
 		set_page_dirty(page);
 		page_cache_release(page);
 	}
+	if (dir->i_mode & S_ISGID)
+		inode->i_gid = dir->i_gid;
 	dir->i_size += BOGO_DIRENT_SIZE;
 	dir->i_ctime = dir->i_mtime = CURRENT_TIME;
 	d_instantiate(dentry, inode);

[PATCH] tmpfs 3/7 swapoff/truncate race

[Hugh Dickins]

On 23 July, Sergey S. Kostyliov <rathamahata@php4.ru> reported a tmpfs
BUG_ON(inode->i_blocks) during swapoff: my last version of the fix to
swapoff/truncate race was inadequate, since I_FREEING might get set or
i_size be reduced (from another cpu) the instant after it's tested here.

So revert to the previous version of the fix, shmem_truncate calling
truncate_inode_pages again, if pages still left in cache; but avoid the
recall in usual cases of partial truncation, by having a "pagein" flag
to indicate when recall might be necessary.  (Since those flags already
use VM_ACCOUNT and VM_LOCKED, must redefine another VM_flag for this.)
Sergey and 2.4-aa have run fine with this for a couple of months.

--- tmpfs2/mm/shmem.c	2003-10-15 15:38:41.336892568 +0100
+++ tmpfs3/mm/shmem.c	2003-10-15 15:38:52.342219504 +0100
@@ -52,6 +52,9 @@
 
 #define VM_ACCT(size)    (PAGE_CACHE_ALIGN(size) >> PAGE_SHIFT)
 
+/* info->flags needs a VM_flag to handle swapoff/truncate races efficiently */
+#define SHMEM_PAGEIN	 VM_READ
+
 /* Pretend that each entry is of this size in directory's i_size */
 #define BOGO_DIRENT_SIZE 20
 
@@ -490,6 +493,16 @@
 	}
 done2:
 	BUG_ON(info->swapped > info->next_index);
+	if (inode->i_mapping->nrpages && (info->flags & SHMEM_PAGEIN)) {
+		/*
+		 * Call truncate_inode_pages again: racing shmem_unuse_inode
+		 * may have swizzled a page in from swap since vmtruncate or
+		 * generic_delete_inode did it, before we lowered next_index.
+		 */
+		spin_unlock(&info->lock);
+		truncate_inode_pages(inode->i_mapping, inode->i_size);
+		spin_lock(&info->lock);
+	}
 	shmem_recalc_inode(inode);
 	spin_unlock(&info->lock);
 }
@@ -524,6 +537,19 @@
 					attr->ia_size>>PAGE_CACHE_SHIFT,
 						&page, SGP_READ);
 			}
+			/*
+			 * Reset SHMEM_PAGEIN flag so that shmem_truncate can
+			 * detect if any pages might have been added to cache
+			 * after truncate_inode_pages.  But we needn't bother
+			 * if it's being fully truncated to zero-length: the
+			 * nrpages check is efficient enough in that case.
+			 */
+			if (attr->ia_size) {
+				struct shmem_inode_info *info = SHMEM_I(inode);
+				spin_lock(&info->lock);
+				info->flags &= ~SHMEM_PAGEIN;
+				spin_unlock(&info->lock);
+			}
 		}
 	}
 
@@ -638,14 +664,10 @@
 found:
 	idx += offset;
 	inode = &info->vfs_inode;
-
-	/* Racing against delete or truncate? Must leave out of page cache */
-	limit = (inode->i_state & I_FREEING)? 0:
-		(i_size_read(inode) + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT;
-
-	if (idx >= limit ||
-	    move_from_swap_cache(page, idx, inode->i_mapping) == 0)
+	if (move_from_swap_cache(page, idx, inode->i_mapping) == 0) {
+		info->flags |= SHMEM_PAGEIN;
 		shmem_swp_set(info, ptr + offset, 0);
+	}
 	shmem_swp_unmap(ptr);
 	spin_unlock(&info->lock);
 	/*
@@ -653,7 +675,7 @@
 	 * try_to_unuse will skip over mms, then reincrement count.
 	 */
 	swap_free(entry);
-	return idx < limit;
+	return 1;
 }
 
 /*

[PATCH] tmpfs 4/7 getpage/truncate race

[Hugh Dickins]

Extend use of that SHMEM_PAGEIN flag to where shmem_getpage adds a page
to the cache.  It couldn't have caused a BUG_ON(inode->i_blocks), but if
i_size is reduced (from another cpu) the instant after shmem_swp_alloc
checks it, shmem_getpage could insert a page into the cache just after
truncate_inode_pages has passed through cleaning it, leaving stale data
(which may mysteriously reappear if the file is later extended).

Easily fixed for tmpfs, using the mechanism just added for swapoff; and
probably more important there, since its read from swap can insert non-0
data.  But is there not a similar issue, a tiny window, in filemap.c?
if truncate_inode_pages comes in between checking i_size and adding new
page to cache.  Not worth getting excited, but something to beware of.

--- tmpfs3/mm/shmem.c	2003-10-15 15:38:52.342219504 +0100
+++ tmpfs4/mm/shmem.c	2003-10-15 15:39:03.328549328 +0100
@@ -52,7 +52,7 @@
 
 #define VM_ACCT(size)    (PAGE_CACHE_ALIGN(size) >> PAGE_SHIFT)
 
-/* info->flags needs a VM_flag to handle swapoff/truncate races efficiently */
+/* info->flags needs a VM_flag to handle pagein/truncate races efficiently */
 #define SHMEM_PAGEIN	 VM_READ
 
 /* Pretend that each entry is of this size in directory's i_size */
@@ -498,6 +498,8 @@
 		 * Call truncate_inode_pages again: racing shmem_unuse_inode
 		 * may have swizzled a page in from swap since vmtruncate or
 		 * generic_delete_inode did it, before we lowered next_index.
+		 * Also, though shmem_getpage checks i_size before adding to
+		 * cache, no recheck after: so fix the narrow window there too.
 		 */
 		spin_unlock(&info->lock);
 		truncate_inode_pages(inode->i_mapping, inode->i_size);
@@ -863,6 +865,7 @@
 			swap_free(swap);
 		} else if (!(error = move_from_swap_cache(
 				swappage, idx, mapping))) {
+			info->flags |= SHMEM_PAGEIN;
 			shmem_swp_set(info, entry, 0);
 			shmem_swp_unmap(entry);
 			spin_unlock(&info->lock);
@@ -932,6 +935,7 @@
 					goto failed;
 				goto repeat;
 			}
+			info->flags |= SHMEM_PAGEIN;
 		}
 
 		info->alloced++;

[PATCH] tmpfs 5/7 writepage/truncate race

[Hugh Dickins]

If it's possible for a tmpfs page beyond i_size to remain in cache until
shmem_truncate repeats truncate_inode_pages, then shmem_writepage's
BUG_ON(index >= info->next_index) cannot be completely safe.  But it's a
useful check in a fragile area, so retain it when not in shmem_truncate.

--- tmpfs4/mm/shmem.c	2003-10-15 15:39:03.328549328 +0100
+++ tmpfs5/mm/shmem.c	2003-10-15 15:39:14.391867448 +0100
@@ -52,8 +52,9 @@
 
 #define VM_ACCT(size)    (PAGE_CACHE_ALIGN(size) >> PAGE_SHIFT)
 
-/* info->flags needs a VM_flag to handle pagein/truncate races efficiently */
+/* info->flags needs VM_flags to handle pagein/truncate races efficiently */
 #define SHMEM_PAGEIN	 VM_READ
+#define SHMEM_TRUNCATE	 VM_WRITE
 
 /* Pretend that each entry is of this size in directory's i_size */
 #define BOGO_DIRENT_SIZE 20
@@ -393,6 +394,7 @@
 		return;
 
 	spin_lock(&info->lock);
+	info->flags |= SHMEM_TRUNCATE;
 	limit = info->next_index;
 	info->next_index = idx;
 	if (info->swapped && idx < SHMEM_NR_DIRECT) {
@@ -505,6 +507,7 @@
 		truncate_inode_pages(inode->i_mapping, inode->i_size);
 		spin_lock(&info->lock);
 	}
+	info->flags &= ~SHMEM_TRUNCATE;
 	shmem_recalc_inode(inode);
 	spin_unlock(&info->lock);
 }
@@ -730,7 +733,10 @@
 
 	spin_lock(&info->lock);
 	shmem_recalc_inode(inode);
-	BUG_ON(index >= info->next_index);
+	if (index >= info->next_index) {
+		BUG_ON(!(info->flags & SHMEM_TRUNCATE));
+		goto unlock;
+	}
 	entry = shmem_swp_entry(info, index, NULL);
 	BUG_ON(!entry);
 	BUG_ON(entry->val);
@@ -744,6 +750,7 @@
 	}
 
 	shmem_swp_unmap(entry);
+unlock:
 	spin_unlock(&info->lock);
 	swap_free(swap);
 redirty:

[PATCH] tmpfs 6/7 write i_size_write

[Hugh Dickins]

mm/shmem.c was converted to i_size_read in -test1, and the remaining
references to a file's naked i_size are safely protected by i_sem;
but surely shmem_file_write must use i_size_write to update i_size.

--- tmpfs5/mm/shmem.c	2003-10-15 15:39:14.391867448 +0100
+++ tmpfs6/mm/shmem.c	2003-10-15 15:39:25.386196056 +0100
@@ -1239,7 +1239,7 @@
 		pos += bytes;
 		buf += bytes;
 		if (pos > inode->i_size)
-			inode->i_size = pos;
+			i_size_write(inode, pos);
 
 		flush_dcache_page(page);
 		set_page_dirty(page);

[PATCH] tmpfs 7/7 write mark_page_accessed

[Hugh Dickins]

mm/filemap.c's generic_file_aio_write_nolock changed SetPageReferenced
to mark_page_accessed in -test3: now follow that in shmem_file_write.

--- tmpfs6/mm/shmem.c	2003-10-15 15:39:25.386196056 +0100
+++ tmpfs7/mm/shmem.c	2003-10-15 15:39:36.380524664 +0100
@@ -1243,8 +1243,7 @@
 
 		flush_dcache_page(page);
 		set_page_dirty(page);
-		if (!PageReferenced(page))
-			SetPageReferenced(page);
+		mark_page_accessed(page);
 		page_cache_release(page);
 
 		if (left) {

Re: [PATCH] tmpfs 2/7 LTP S_ISGID dir

[viro]

On Wed, Oct 15, 2003 at 07:19:46PM +0100, Hugh Dickins wrote:
> LTP tests the filesystem on /tmp: many failures when tmpfs because
> it missed the way giddy directories hand down their gid.  Also fix
> ramfs and hugetlbfs.

*the* way?  I can think of at least two...

Re: [PATCH] tmpfs 2/7 LTP S_ISGID dir

[Hugh Dickins]

On Wed, 15 Oct 2003 viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Wed, Oct 15, 2003 at 07:19:46PM +0100, Hugh Dickins wrote:
> > LTP tests the filesystem on /tmp: many failures when tmpfs because
> > it missed the way giddy directories hand down their gid.  Also fix
> > ramfs and hugetlbfs.
> 
> *the* way?  I can think of at least two...

You mean, the way they do directories and the way they do non-directories?
Or, the way they do it if they do it, and the way they do it if they don't?
Or something else?  Please, share your thought!

Thanks,
Hugh

Re: [PATCH] tmpfs 2/7 LTP S_ISGID dir

[viro]

On Wed, Oct 15, 2003 at 08:48:59PM +0100, Hugh Dickins wrote:
> On Wed, 15 Oct 2003 viro@parcelfarce.linux.theplanet.co.uk wrote:
> > On Wed, Oct 15, 2003 at 07:19:46PM +0100, Hugh Dickins wrote:
> > > LTP tests the filesystem on /tmp: many failures when tmpfs because
> > > it missed the way giddy directories hand down their gid.  Also fix
> > > ramfs and hugetlbfs.
> > 
> > *the* way?  I can think of at least two...
> 
> You mean, the way they do directories and the way they do non-directories?
> Or, the way they do it if they do it, and the way they do it if they don't?
> Or something else?  Please, share your thought!

"We always inherit parents gid, sgid is ignored" and "we do that only
if parent is sgid and children that happen to be directories inherit
sgid from parent".  Yes, ramfs et.al. follow neither of those, but which
way to change that is an interesting question...

Re: [PATCH] tmpfs 2/7 LTP S_ISGID dir

[Hugh Dickins]

On Wed, 15 Oct 2003 viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Wed, Oct 15, 2003 at 08:48:59PM +0100, Hugh Dickins wrote:
> > On Wed, 15 Oct 2003 viro@parcelfarce.linux.theplanet.co.uk wrote:
> > > On Wed, Oct 15, 2003 at 07:19:46PM +0100, Hugh Dickins wrote:
> > > > LTP tests the filesystem on /tmp: many failures when tmpfs because
> > > > it missed the way giddy directories hand down their gid.  Also fix
> > > > ramfs and hugetlbfs.
> > > 
> > > *the* way?  I can think of at least two...
> > 
> > You mean, the way they do directories and the way they do non-directories?
> > Or, the way they do it if they do it, and the way they do it if they don't?
> > Or something else?  Please, share your thought!
> 
> "We always inherit parents gid, sgid is ignored" and "we do that only
> if parent is sgid and children that happen to be directories inherit
> sgid from parent".  Yes, ramfs et.al. follow neither of those, but which
> way to change that is an interesting question...

The patch chooses the second, which I see as merely fixing an oversight
(in the exceptional S_ISGID case); to choose the first convention would
be a significant change in behaviour (or a feature request for the option).

Hugh