Re: Kernel bug: Bad page state: related to generic symlink code andmmap

[Anton Altaparmakov]

On Fri, 19 Aug 2005, Linus Torvalds wrote:
> The one thing that strikes me is that we might actually have less pain if
> we just changed the calling convention for follow_link/put_link slightly
> instead of creating a new library function. The existing "page cache"  
> thing really _does_ work very well, and would work fine for NFS and ncpfs
> too, if we just allowed an extra cookie to be passed along from
> "follow_link()" to "put_link()".
> 
> A patch like this (totally untested, and you'd need to update any
> filesystems that don't use the regular page_follow_link interface) would
> seem to clean up the mess nicely.. The basic change is that follow_link() 
> returns a error-pointer cookie instead of just zero or error, and that is 
> passed into put_link().
> 
> That simple calling convention change solves all problems. It so _happens_ 
> that any old binary code also continues to work (the cookie will be zero, 
> and put_link will ignore it), so it shouldn't even break any unconverted 
> stuff (unless they mix using their own functions _and_ using the helpher 
> functions, which is of course possible).
> 
> The "shouldn't break nonconverted filesystems" makes me think this is a 
> safe change. Comments?
> 
> NOTE NOTE NOTE! Let me say again that it's untested. It might not break 
> nonconverted filesystems, but it equally well migth break even the 
> converted ones ;)
> 
> 		Linus
> 
> ----
> diff --git a/fs/namei.c b/fs/namei.c
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -501,6 +501,7 @@ struct path {
>  static inline int __do_follow_link(struct path *path, struct nameidata *nd)
>  {
>  	int error;
> +	void *cookie;
>  	struct dentry *dentry = path->dentry;
>  
>  	touch_atime(path->mnt, dentry);
> @@ -508,13 +509,15 @@ static inline int __do_follow_link(struc
>  
>  	if (path->mnt == nd->mnt)
>  		mntget(path->mnt);
> -	error = dentry->d_inode->i_op->follow_link(dentry, nd);
> -	if (!error) {
> +	cookie = dentry->d_inode->i_op->follow_link(dentry, nd);
> +	error = PTR_ERR(cookie);
> +	if (!IS_ERR(cookie)) {
>  		char *s = nd_get_link(nd);
> +		error = 0;
>  		if (s)
>  			error = __vfs_follow_link(nd, s);
>  		if (dentry->d_inode->i_op->put_link)
> -			dentry->d_inode->i_op->put_link(dentry, nd);
> +			dentry->d_inode->i_op->put_link(dentry, nd, cookie);
>  	}
>  	dput(dentry);
>  	mntput(path->mnt);
> @@ -2345,14 +2348,17 @@ int generic_readlink(struct dentry *dent
>  {
>  	struct nameidata nd;
>  	int res;
> +	void *cookie;
> +
>  	nd.depth = 0;
> -	res = dentry->d_inode->i_op->follow_link(dentry, &nd);
> -	if (!res) {
> +	cookie = dentry->d_inode->i_op->follow_link(dentry, &nd);
> +	if (!IS_ERR(cookie)) {
>  		res = vfs_readlink(dentry, buffer, buflen, nd_get_link(&nd));
>  		if (dentry->d_inode->i_op->put_link)
> -			dentry->d_inode->i_op->put_link(dentry, &nd);
> +			dentry->d_inode->i_op->put_link(dentry, &nd, cookie);
> +		cookie = ERR_PTR(0);

You are throwing away the error return from vfs_readlink().  I suspect you 
wanted:

+		cookie = ERR_PTR(res);

>  	}
> -	return res;
> +	return PTR_ERR(cookie);
>  }
>  
>  int vfs_follow_link(struct nameidata *nd, const char *link)
> @@ -2395,23 +2401,19 @@ int page_readlink(struct dentry *dentry,
>  	return res;
>  }
>  
> -int page_follow_link_light(struct dentry *dentry, struct nameidata *nd)
> +void *page_follow_link_light(struct dentry *dentry, struct nameidata *nd)
>  {
>  	struct page *page;
>  	nd_set_link(nd, page_getlink(dentry, &page));
> -	return 0;
> +	return page;
>  }
>  
> -void page_put_link(struct dentry *dentry, struct nameidata *nd)
> +void page_put_link(struct dentry *dentry, struct nameidata *nd, void *cookie)
>  {
>  	if (!IS_ERR(nd_get_link(nd))) {
> -		struct page *page;
> -		page = find_get_page(dentry->d_inode->i_mapping, 0);
> -		if (!page)
> -			BUG();
> +		struct page *page = cookie;
>  		kunmap(page);
>  		page_cache_release(page);
> -		page_cache_release(page);
>  	}
>  }
>  
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -993,8 +993,8 @@ struct inode_operations {
>  	int (*rename) (struct inode *, struct dentry *,
>  			struct inode *, struct dentry *);
>  	int (*readlink) (struct dentry *, char __user *,int);
> -	int (*follow_link) (struct dentry *, struct nameidata *);
> -	void (*put_link) (struct dentry *, struct nameidata *);
> +	void * (*follow_link) (struct dentry *, struct nameidata *);
> +	void (*put_link) (struct dentry *, struct nameidata *, void *);
>  	void (*truncate) (struct inode *);
>  	int (*permission) (struct inode *, int, struct nameidata *);
>  	int (*setattr) (struct dentry *, struct iattr *);
> @@ -1602,8 +1602,8 @@ extern struct file_operations generic_ro
>  extern int vfs_readlink(struct dentry *, char __user *, int, const char *);
>  extern int vfs_follow_link(struct nameidata *, const char *);
>  extern int page_readlink(struct dentry *, char __user *, int);
> -extern int page_follow_link_light(struct dentry *, struct nameidata *);
> -extern void page_put_link(struct dentry *, struct nameidata *);
> +extern void *page_follow_link_light(struct dentry *, struct nameidata *);
> +extern void page_put_link(struct dentry *, struct nameidata *, void *);
>  extern int page_symlink(struct inode *inode, const char *symname, int len);
>  extern struct inode_operations page_symlink_inode_operations;
>  extern int generic_readlink(struct dentry *, char __user *, int);

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/