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

Groups > linux.kernel > #1288072 > 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> - 2015-12-10 01:20 +0100
    Re: linux-next: build failure after merge of the vfs tree Stephen Rothwell <sfr@canb.auug.org.au> - 2015-12-10 01:30 +0100
      Re: linux-next: build failure after merge of the vfs tree Al Viro <viro@ZenIV.linux.org.uk> - 2015-12-10 01:50 +0100
        Re: linux-next: build failure after merge of the vfs tree Mike Marshall <hubcap@omnibond.com> - 2015-12-10 16:50 +0100

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

Hi Al,

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

fs/orangefs/symlink.c:26:2: error: unknown field 'follow_link' specified in initializer
  .follow_link = pvfs2_follow_link,
  ^
fs/orangefs/symlink.c:26:17: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
  .follow_link = pvfs2_follow_link, 
                 ^
fs/orangefs/symlink.c:26:17: note: (near initialization for 'pvfs2_symlink_inode_operations.put_link')

Caused by commit

  6b2553918d8b ("replace ->follow_link() with new method that could stay in RCU mode")

[I wish there was some way to stage these API changes :-(]

I applied the following merge fix patch (which may need more work):

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Thu, 10 Dec 2015 11:12:36 +1100
Subject: [PATCH] orangfs: update for follow_link to get_link change

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 fs/orangefs/symlink.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/orangefs/symlink.c b/fs/orangefs/symlink.c
index 2adfceff7730..dbf24a98a3c9 100644
--- a/fs/orangefs/symlink.c
+++ b/fs/orangefs/symlink.c
@@ -8,9 +8,15 @@
 #include "pvfs2-kernel.h"
 #include "pvfs2-bufmap.h"
 
-static const char *pvfs2_follow_link(struct dentry *dentry, void **cookie)
+static const char *pvfs2_get_link(struct dentry *dentry, struct inode *inode,
+				  void **cookie)
 {
-	char *target =  PVFS2_I(dentry->d_inode)->link_target;
+	char *target;
+
+	if (!dentry)
+		return ERR_PTR(-ECHILD);
+
+	target =  PVFS2_I(inode)->link_target;
 
 	gossip_debug(GOSSIP_INODE_DEBUG,
 		     "%s: called on %s (target is %p)\n",
@@ -23,7 +29,7 @@ static const char *pvfs2_follow_link(struct dentry *dentry, void **cookie)
 
 struct inode_operations pvfs2_symlink_inode_operations = {
 	.readlink = generic_readlink,
-	.follow_link = pvfs2_follow_link,
+	.get_link = pvfs2_get_link,
 	.setattr = pvfs2_setattr,
 	.getattr = pvfs2_getattr,
 	.listxattr = pvfs2_listxattr,
-- 
2.6.2

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
--
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/

[toc] | [next] | [standalone]

#1288079

[Just adding the origefs maintainer to the cc list]

On Thu, 10 Dec 2015 11:18:47 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Hi Al,
> 
> After merging the vfs tree, today's linux-next build (x86_64 allmodconfig)
> failed like this:
> 
> fs/orangefs/symlink.c:26:2: error: unknown field 'follow_link' specified in initializer
>   .follow_link = pvfs2_follow_link,
>   ^
> fs/orangefs/symlink.c:26:17: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
>   .follow_link = pvfs2_follow_link, 
>                  ^
> fs/orangefs/symlink.c:26:17: note: (near initialization for 'pvfs2_symlink_inode_operations.put_link')
> 
> Caused by commit
> 
>   6b2553918d8b ("replace ->follow_link() with new method that could stay in RCU mode")
> 
> [I wish there was some way to stage these API changes :-(]
> 
> I applied the following merge fix patch (which may need more work):
> 
> From: Stephen Rothwell <sfr@canb.auug.org.au>
> Date: Thu, 10 Dec 2015 11:12:36 +1100
> Subject: [PATCH] orangfs: update for follow_link to get_link change
> 
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> ---
>  fs/orangefs/symlink.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/orangefs/symlink.c b/fs/orangefs/symlink.c
> index 2adfceff7730..dbf24a98a3c9 100644
> --- a/fs/orangefs/symlink.c
> +++ b/fs/orangefs/symlink.c
> @@ -8,9 +8,15 @@
>  #include "pvfs2-kernel.h"
>  #include "pvfs2-bufmap.h"
>  
> -static const char *pvfs2_follow_link(struct dentry *dentry, void **cookie)
> +static const char *pvfs2_get_link(struct dentry *dentry, struct inode *inode,
> +				  void **cookie)
>  {
> -	char *target =  PVFS2_I(dentry->d_inode)->link_target;
> +	char *target;
> +
> +	if (!dentry)
> +		return ERR_PTR(-ECHILD);
> +
> +	target =  PVFS2_I(inode)->link_target;
>  
>  	gossip_debug(GOSSIP_INODE_DEBUG,
>  		     "%s: called on %s (target is %p)\n",
> @@ -23,7 +29,7 @@ static const char *pvfs2_follow_link(struct dentry *dentry, void **cookie)
>  
>  struct inode_operations pvfs2_symlink_inode_operations = {
>  	.readlink = generic_readlink,
> -	.follow_link = pvfs2_follow_link,
> +	.get_link = pvfs2_get_link,
>  	.setattr = pvfs2_setattr,
>  	.getattr = pvfs2_getattr,
>  	.listxattr = pvfs2_listxattr,
> -- 
> 2.6.2

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
--
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/

[toc] | [prev] | [next] | [standalone]

#1288090

On Thu, Dec 10, 2015 at 11:23:22AM +1100, Stephen Rothwell wrote:
> [Just adding the origefs maintainer to the cc list]
> > -static const char *pvfs2_follow_link(struct dentry *dentry, void **cookie)
> > +static const char *pvfs2_get_link(struct dentry *dentry, struct inode *inode,
> > +				  void **cookie)
> >  {
> > -	char *target =  PVFS2_I(dentry->d_inode)->link_target;

Better fix is to have inode->link = PVFS2_I(dentry->d_inode)->link_target;
when we set the latter and use .get_link = simple_get_link...

Said that, there is an unpleasant bug in that area - link_target of a live
inode can be overwritten, right under the pathname resolution walking the
old contents of that thing.

copy_attributes_to_inode() is triggered by ->d_revalidate() and by ->getattr()
and it's really, really unsafe for a live inode.  Just look what it does
to ->i_mode...  Sure, normally a server won't return different symlink bodies
on subsequent getattr requests.  As long as it's sane (and not compromised,
etc.), but relying upon that is not a good idea.
--
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/

[toc] | [prev] | [next] | [standalone]

#1288617

 > Said that, there is an unpleasant bug in that area - link_target of a live
 > inode can be overwritten, right under the pathname resolution walking the
 > old contents of that thing

Figuring that out is on the list. This week I've been working on cleaning up
orangefs_devreq_writev, and Martin even has a version that changes
the protocol where userspace uses write instead of writev, getting rid
of the 4-or-5 iovec scheme Al hates. If he still hates it after the code
is readable, we'll probably go that direction...

And we have an infant fuzzer that we've already crashed the kernel with (and
made an easy and good fix, I think).

And also this week I have tried to address Linus' concerns about our
old fashioned waiting scheme where we used add_wait_queue and
set_current_state instead of the wait_event() model.

Yi Liu who is also working with us on this project has provided a patch
that changes all the pvfs2 occurrences to orangefs.

These patches will be in our kernel.org tree very soon I hope,
but we won't be done yet...

-Mike "you're never done..."

On Wed, Dec 9, 2015 at 7:48 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Dec 10, 2015 at 11:23:22AM +1100, Stephen Rothwell wrote:
>> [Just adding the origefs maintainer to the cc list]
>> > -static const char *pvfs2_follow_link(struct dentry *dentry, void **cookie)
>> > +static const char *pvfs2_get_link(struct dentry *dentry, struct inode *inode,
>> > +                             void **cookie)
>> >  {
>> > -   char *target =  PVFS2_I(dentry->d_inode)->link_target;
>
> Better fix is to have inode->link = PVFS2_I(dentry->d_inode)->link_target;
> when we set the latter and use .get_link = simple_get_link...
>
> Said that, there is an unpleasant bug in that area - link_target of a live
> inode can be overwritten, right under the pathname resolution walking the
> old contents of that thing.
>
> copy_attributes_to_inode() is triggered by ->d_revalidate() and by ->getattr()
> and it's really, really unsafe for a live inode.  Just look what it does
> to ->i_mode...  Sure, normally a server won't return different symlink bodies
> on subsequent getattr requests.  As long as it's sane (and not compromised,
> etc.), but relying upon that is not a good idea.
--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web