Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1452199 > unrolled thread

linux-next: build failure after merge of the vfs tree

Back to article view | Back to linux.kernel

Contents

  linux-next: build failure after merge of the vfs tree Stephen Rothwell <sfr@canb.auug.org.au> - 2016-07-29 03:20 +0200
    Re: linux-next: build failure after merge of the vfs tree Al Viro <viro@ZenIV.linux.org.uk> - 2016-07-29 06:20 +0200

#1452199 — linux-next: build failure after merge of the vfs tree

Hi Al,

After merging the vfs tree, today's linux-next build (powerpc
ppc64_defconfig) failed like this:

fs/fuse/dir.c: In function 'fuse_reverse_inval_entry':
fs/fuse/dir.c:958:13: error: assignment of member 'hash' in read-only object
  name->hash = full_name_hash(dir, name->name, name->len);
             ^

Caused by commit

  8387ff2577eb ("vfs: make the string hashes salt the hash")

from Linus' tree interacting with commit

  5e70178ae20b ("qstr: constify instances in fuse")

from the vfs tree.

I added this merge fix patch:

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Fri, 29 Jul 2016 11:07:45 +1000
Subject: [PATCH] qstr: unconstify fuse_reverse_inval_entry parameter

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 fs/fuse/dir.c    | 2 +-
 fs/fuse/fuse_i.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index f910578e51ba..c47b7780ce37 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -935,7 +935,7 @@ int fuse_update_attributes(struct inode *inode, struct kstat *stat,
 }
 
 int fuse_reverse_inval_entry(struct super_block *sb, u64 parent_nodeid,
-			     u64 child_nodeid, const struct qstr *name)
+			     u64 child_nodeid, struct qstr *name)
 {
 	int err = -ENOTDIR;
 	struct inode *parent;
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 6df761726a53..d98d8cc84def 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -929,7 +929,7 @@ int fuse_reverse_inval_inode(struct super_block *sb, u64 nodeid,
  * then the dentry is unhashed (d_delete()).
  */
 int fuse_reverse_inval_entry(struct super_block *sb, u64 parent_nodeid,
-			     u64 child_nodeid, const struct qstr *name);
+			     u64 child_nodeid, struct qstr *name);
 
 int fuse_do_open(struct fuse_conn *fc, u64 nodeid, struct file *file,
 		 bool isdir);
-- 
2.8.1

-- 
Cheers,
Stephen Rothwell

[toc] | [next] | [standalone]

#1452244

On Fri, Jul 29, 2016 at 11:19:38AM +1000, Stephen Rothwell wrote:
> ---
>  fs/fuse/dir.c    | 2 +-
>  fs/fuse/fuse_i.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index f910578e51ba..c47b7780ce37 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -935,7 +935,7 @@ int fuse_update_attributes(struct inode *inode, struct kstat *stat,
>  }
>  
>  int fuse_reverse_inval_entry(struct super_block *sb, u64 parent_nodeid,
> -			     u64 child_nodeid, const struct qstr *name)
> +			     u64 child_nodeid, struct qstr *name)
>  {
>  	int err = -ENOTDIR;
>  	struct inode *parent;

I'm not sure if it's the best way to handle that, TBH...  It might be better
to pass name.name/name.len separately here.  Both callers have a _lot_ of
code duplication; I've a patch getting rid of code duplication there, will
play with it and see if it would make sense to quit messing with struct
qstr while we are at it.

BTW, I'd been toying with the following trick:

static inline const struct qstr *d_name(const struct dentry *dentry)
{
	return &dentry->d_name;
}
with subsequent switch of dentry->d_name.foo to d_name(dentry)->foo and
&dentry->d_name to d_name(dentry).  Note 'const' in the above - the point is,
there are very few places where dentry->d_name can be legitimately modified
(__d_alloc(), swap_names() and copy_name()) and it'd be nice to have cc(1)
enforce that.  Changing d_name to const struct qstr (and explicitly casting
in the aforementioned 3 functions) would do it, but it's deep in nasal daemon
territory; OTOH, conversion to the helper above with subsequent renaming of
the field to something easily greppable for would get the same effect and
stay within standard C.

FWIW, the whole "constify struct qstr * arguments" series is due to hunting
for ppc bug reported a while ago; it manifested as NULL ->d_name.name observed
in __d_lookup_rcu().  AFAICS, it's an effect of earlier memory corruption,
seeing that there was list_del() in prune_dcache_sb() hitting NULL ->prev->next
(in __list_del_entry(), probably via prune_dcache_sb()->shrink_dentry_list()->
d_shrink_del()->list_del_init(&dentry->d_lru)), but it would be nice to have
an easier way to prove that nothing would be able to bugger ->d_name.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web