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 <[email protected]>
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 <[email protected]>
---
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 [email protected]
[Just adding the origefs maintainer to the cc list]
On Thu, 10 Dec 2015 11:18:47 +1100 Stephen Rothwell <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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 [email protected]
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.
> 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 <[email protected]> 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.
Hi Stephen,
On Thu, 10 Dec 2015 11:18:47 +1100 Stephen Rothwell <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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,
This patch now looks like this (after changes to the orangefs tree):
diff --git a/fs/orangefs/symlink.c b/fs/orangefs/symlink.c
index 1b3ae63463dc..01977e88e95d 100644
--- a/fs/orangefs/symlink.c
+++ b/fs/orangefs/symlink.c
@@ -8,9 +8,15 @@
#include "orangefs-kernel.h"
#include "orangefs-bufmap.h"
-static const char *orangefs_follow_link(struct dentry *dentry, void **cookie)
+static const char *orangefs_get_link(struct dentry *dentry, struct inode *inode,
+ void **cookie)
{
- char *target = ORANGEFS_I(dentry->d_inode)->link_target;
+ char *target;
+
+ if (!dentry)
+ return ERR_PTR(-ECHILD);
+
+ target = ORANGEFS_I(inode)->link_target;
gossip_debug(GOSSIP_INODE_DEBUG,
"%s: called on %s (target is %p)\n",
@@ -23,7 +29,7 @@ static const char *orangefs_follow_link(struct dentry *dentry, void **cookie)
struct inode_operations orangefs_symlink_inode_operations = {
.readlink = generic_readlink,
- .follow_link = orangefs_follow_link,
+ .get_link = orangefs_get_link,
.setattr = orangefs_setattr,
.getattr = orangefs_getattr,
.listxattr = orangefs_listxattr,
--
Cheers,
Stephen Rothwell [email protected]