[PATCH] ceph: Fix oops when mounting cephfs with non-existing path

[PATCH] ceph: Fix oops when mounting cephfs with non-existing path

[Yan, Zheng]

From: "Yan, Zheng" <zheng.z.yan@intel.com>

req->r_locked_dir is NULL if the request was created by
open_root_dentry(). ceph_fill_trace() lacks check for this case.
It causes oops when mounting cephfs with non-existing path.

Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
---
 fs/ceph/inode.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 9fff9f3..40e2ef1 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -992,6 +992,13 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req,
 	if (rinfo->head->is_dentry) {
 		struct inode *dir = req->r_locked_dir;
 
+		/*
+		 * req->r_locked_dir is null if the request was created
+		 * by open_root_dentry()
+		 */
+		if (!dir)
+			return le32_to_cpu(rinfo->head->result);
+
 		err = fill_inode(dir, &rinfo->diri, rinfo->dirfrag,
 				 session, req->r_request_started, -1,
 				 &req->r_caps_reservation);
-- 
1.7.11.2

Re: [PATCH] ceph: Fix oops when mounting cephfs with non-existing path

[Sage Weil]

Hi Yan,

On Fri, 17 Aug 2012, Yan, Zheng wrote:
> From: "Yan, Zheng" <zheng.z.yan@intel.com>
> 
> req->r_locked_dir is NULL if the request was created by
> open_root_dentry(). ceph_fill_trace() lacks check for this case.
> It causes oops when mounting cephfs with non-existing path.

This is actually an MDS bug.  The problem is that the client is issuing a 
GETATTR request and isn't prepared to handle a dentry in the reply, but 
the MDS sees that there is a relative path in the request and behaves like 
this is a lookup.  We were distinguishing correctly between the two cases 
on success, but not on error.  I've pushed branch bug-2969 to ceph.git 
which fixes this for me.  Can you please test and verify it resolves 
the problem?

We may want to have a check like the below, but it should probably be 
accompanied by a pr_warning or WARN_ON() since it is a protocol error.  
Generally speaking, the client isn't allowed to ignore state the MDS has 
issued it (in this case, caps on the directory inode; lower down in 
fill_trace possibly a dentry lease) without messing up the protocol.

Thanks for looking at this!  Looking at your global_id patch next...

Thanks-
sage


http://tracker.newdream.net/issues/2959

> 
> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
> ---
>  fs/ceph/inode.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> index 9fff9f3..40e2ef1 100644
> --- a/fs/ceph/inode.c
> +++ b/fs/ceph/inode.c
> @@ -992,6 +992,13 @@ int ceph_fill_trace(struct super_block *sb, struct ceph_mds_request *req,
>  	if (rinfo->head->is_dentry) {
>  		struct inode *dir = req->r_locked_dir;
>  
> +		/*
> +		 * req->r_locked_dir is null if the request was created
> +		 * by open_root_dentry()
> +		 */
> +		if (!dir)
> +			return le32_to_cpu(rinfo->head->result);
> +
>  		err = fill_inode(dir, &rinfo->diri, rinfo->dirfrag,
>  				 session, req->r_request_started, -1,
>  				 &req->r_caps_reservation);
> -- 
> 1.7.11.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH] ceph: Fix oops when mounting cephfs with non-existing path

[Tommi Virtanen]

On Fri, Aug 17, 2012 at 8:54 AM, Sage Weil <sage@inktank.com> wrote:
>> req->r_locked_dir is NULL if the request was created by
>> open_root_dentry(). ceph_fill_trace() lacks check for this case.
>> It causes oops when mounting cephfs with non-existing path.
> This is actually an MDS bug.  The problem is that the client is issuing a

It's also a kernel bug -- there's no excuse for an oops, even if the
server spoke pig latin.

Re: [PATCH] ceph: Fix oops when mounting cephfs with non-existing path

[Yan, Zheng]

On 08/17/2012 11:54 PM, Sage Weil wrote:
> Hi Yan,
> 
> On Fri, 17 Aug 2012, Yan, Zheng wrote:
>> From: "Yan, Zheng" <zheng.z.yan@intel.com>
>>
>> req->r_locked_dir is NULL if the request was created by
>> open_root_dentry(). ceph_fill_trace() lacks check for this case.
>> It causes oops when mounting cephfs with non-existing path.
> 
> This is actually an MDS bug.  The problem is that the client is issuing a 
> GETATTR request and isn't prepared to handle a dentry in the reply, but 
> the MDS sees that there is a relative path in the request and behaves like 
> this is a lookup.  We were distinguishing correctly between the two cases 
> on success, but not on error.  I've pushed branch bug-2969 to ceph.git 
> which fixes this for me.  Can you please test and verify it resolves 
> the problem?
It does resolve the problem.

> 
> We may want to have a check like the below, but it should probably be 
> accompanied by a pr_warning or WARN_ON() since it is a protocol error.  
> Generally speaking, the client isn't allowed to ignore state the MDS has 
> issued it (in this case, caps on the directory inode; lower down in 
> fill_trace possibly a dentry lease) without messing up the protocol.
> 

Thank you for your explanation. 

Yan, Zheng