Skip to contentSkip to conversation

nfsd4: handle failure to find backchannel

PATCH In review J. Bruce Fields 2 participants 6 messages Jul 11, 2014 – Jul 18, 2014

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

J. Bruce Fields

author
#0 lore raw

PATCH 1/1 nfsd4: handle failure to find backchannel

The local variable "ses" will be left NULL here in the case we fail to find a connection. Spotted by a coverity scan.

Signed-off-by J. Bruce Fields <[email protected]>
fs/nfsd/nfs4callback.c +8 -4
index 2c73cae9899d..fe22cd5c42d3 100644
@@ -1001,14 +1001,18 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
 	}
 	spin_unlock(&clp->cl_lock);
 
+	if (!c)
+		goto out_no_connection;
 	err = setup_callback_client(clp, &conn, ses);
-	if (err) {
-		nfsd4_mark_cb_down(clp, err);
-		return;
-	}
+	if (err)
+		goto out_no_connection;
 	/* Yay, the callback channel's back! Restart any callbacks: */
 	list_for_each_entry(cb, &clp->cl_callbacks, cb_per_client)
 		run_nfsd4_cb(cb);
+	return;
+out_no_connection:
+	nfsd4_mark_cb_down(clp, err);
+	return;
 }
 
 static void nfsd4_do_callback_rpc(struct work_struct *w)
#1 lore raw
Quoted 10 lines · J. Bruce Fields

The local variable "ses" will be left NULL here in the case we fail to find a connection. Spotted by a coverity scan.

Signed-off-by J. Bruce Fields <[email protected]>
fs/nfsd/nfs4callback.c +0 -0
index 2c73cae9899d..fe22cd5c42d3 100644

fs/nfsd/nfs4callback.c static void nfsd4_process_cb_update(struct nfsd4_callback *cb) · line 1005

Quoted 5 lines
fs/nfsd/nfs4callback.c +2 -0
@@ -1001,14 +1001,18 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
 	}
 	spin_unlock(&clp->cl_lock);
 
+	if (!c)
+		goto out_no_connection;

Setting err to -EINVAL maybe better.
Otherwise, nfsd4_mark_cb_down will be called with err == 0.

fs/nfsd/nfs4callback.c static void nfsd4_process_cb_update(struct nfsd4_callback *cb) · line 1006

Quoted 1 line
fs/nfsd/nfs4callback.c +0 -0
 	err = setup_callback_client(clp, &conn, ses);

setup_callback_client also return -EINVAL when ses == NULL with conn.cb_xprt == NULL.

thanks,
Kinglong Mee

Quoted 21 lines
fs/nfsd/nfs4callback.c +6 -5
-	if (err) {
-		nfsd4_mark_cb_down(clp, err);
-		return;
-	}
+	if (err)
+		goto out_no_connection;
 	/* Yay, the callback channel's back! Restart any callbacks: */
 	list_for_each_entry(cb, &clp->cl_callbacks, cb_per_client)
 		run_nfsd4_cb(cb);
+	return;
+out_no_connection:
+	nfsd4_mark_cb_down(clp, err);
+	return;
 }
 
 static void nfsd4_do_callback_rpc(struct work_struct *w)
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#2 lore raw
Quoted 20 lines · Kinglong Mee
Quoted 10 lines · J. Bruce Fields

The local variable "ses" will be left NULL here in the case we fail to find a connection. Spotted by a coverity scan.

Signed-off-by J. Bruce Fields <[email protected]>
fs/nfsd/nfs4callback.c +0 -0
index 2c73cae9899d..fe22cd5c42d3 100644

fs/nfsd/nfs4callback.c static void nfsd4_process_cb_update(struct nfsd4_callback *cb) · line 1005

Quoted 5 lines
fs/nfsd/nfs4callback.c +2 -0
@@ -1001,14 +1001,18 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
 	}
 	spin_unlock(&clp->cl_lock);
 
+	if (!c)
+		goto out_no_connection;

Setting err to -EINVAL maybe better.
Otherwise, nfsd4_mark_cb_down will be called with err == 0.

Quoted 3 lines

fs/nfsd/nfs4callback.c static void nfsd4_process_cb_update(struct nfsd4_callback *cb) · line 1006

Quoted 1 line
fs/nfsd/nfs4callback.c +0 -0
 	err = setup_callback_client(clp, &conn, ses);

setup_callback_client also return -EINVAL when ses == NULL with conn.cb_xprt == NULL.

Thanks, yes, after looking over this carefully I don't believe we can call setup_callback_client with ses NULL but conn->cb_xprt non-NULL, so this is just a false positive from coverity.

Thanks for the review!

--b.

Quoted 25 lines

thanks,
Kinglong Mee

Quoted 21 lines
fs/nfsd/nfs4callback.c +6 -5
-	if (err) {
-		nfsd4_mark_cb_down(clp, err);
-		return;
-	}
+	if (err)
+		goto out_no_connection;
 	/* Yay, the callback channel's back! Restart any callbacks: */
 	list_for_each_entry(cb, &clp->cl_callbacks, cb_per_client)
 		run_nfsd4_cb(cb);
+	return;
+out_no_connection:
+	nfsd4_mark_cb_down(clp, err);
+	return;
 }
 
 static void nfsd4_do_callback_rpc(struct work_struct *w)
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
Quoted 21 lines · J. Bruce Fields
Quoted 20 lines · Kinglong Mee
Quoted 10 lines · J. Bruce Fields

The local variable "ses" will be left NULL here in the case we fail to find a connection. Spotted by a coverity scan.

Signed-off-by J. Bruce Fields <[email protected]>
fs/nfsd/nfs4callback.c +0 -0
index 2c73cae9899d..fe22cd5c42d3 100644

fs/nfsd/nfs4callback.c static void nfsd4_process_cb_update(struct nfsd4_callback *cb) · line 1005

Quoted 5 lines
fs/nfsd/nfs4callback.c +2 -0
@@ -1001,14 +1001,18 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
 	}
 	spin_unlock(&clp->cl_lock);
 
+	if (!c)
+		goto out_no_connection;

Setting err to -EINVAL maybe better.
Otherwise, nfsd4_mark_cb_down will be called with err == 0.

Quoted 7 lines
Quoted 3 lines

fs/nfsd/nfs4callback.c static void nfsd4_process_cb_update(struct nfsd4_callback *cb) · line 1006

Quoted 1 line
fs/nfsd/nfs4callback.c +0 -0
 	err = setup_callback_client(clp, &conn, ses);

setup_callback_client also return -EINVAL when ses == NULL with conn.cb_xprt == NULL.

Thanks, yes, after looking over this carefully I don't believe we can call setup_callback_client with ses NULL but conn->cb_xprt non-NULL, so this is just a false positive from coverity.

ses and conn->cb_xprt will be set in the same condition before calling setup_callback_client,

996 c = __nfsd4_find_backchannel(clp);
997 if (c) {

 998                 svc_xprt_get(c->cn_xprt);
 999                 conn.cb_xprt = c->cn_xprt;
1000                 ses = c->cn_session;

1001 }
1002 spin_unlock(&clp->cl_lock);
1003
1004 err = setup_callback_client(clp, &conn, ses);

ses and conn->cb_xprt will be NULL or non-NULL in the same time,
so that, call setup_calback_client with ses NULL but conn->cb_xprt non-NULL will not appear.

thanks,
Kinglong Mee

Quoted 22 lines
Quoted 21 lines
Quoted 21 lines
fs/nfsd/nfs4callback.c +6 -5
-	if (err) {
-		nfsd4_mark_cb_down(clp, err);
-		return;
-	}
+	if (err)
+		goto out_no_connection;
 	/* Yay, the callback channel's back! Restart any callbacks: */
 	list_for_each_entry(cb, &clp->cl_callbacks, cb_per_client)
 		run_nfsd4_cb(cb);
+	return;
+out_no_connection:
+	nfsd4_mark_cb_down(clp, err);
+	return;
 }
 
 static void nfsd4_do_callback_rpc(struct work_struct *w)
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#4 lore raw
Quoted 38 lines · Kinglong Mee
Quoted 21 lines · J. Bruce Fields
Quoted 20 lines · Kinglong Mee
Quoted 10 lines · J. Bruce Fields

The local variable "ses" will be left NULL here in the case we fail to find a connection. Spotted by a coverity scan.

Signed-off-by J. Bruce Fields <[email protected]>
fs/nfsd/nfs4callback.c +0 -0
index 2c73cae9899d..fe22cd5c42d3 100644

fs/nfsd/nfs4callback.c static void nfsd4_process_cb_update(struct nfsd4_callback *cb) · line 1005

Quoted 5 lines
fs/nfsd/nfs4callback.c +2 -0
@@ -1001,14 +1001,18 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
 	}
 	spin_unlock(&clp->cl_lock);
 
+	if (!c)
+		goto out_no_connection;

Setting err to -EINVAL maybe better.
Otherwise, nfsd4_mark_cb_down will be called with err == 0.

Quoted 7 lines
Quoted 3 lines

fs/nfsd/nfs4callback.c static void nfsd4_process_cb_update(struct nfsd4_callback *cb) · line 1006

Quoted 1 line
fs/nfsd/nfs4callback.c +0 -0
 	err = setup_callback_client(clp, &conn, ses);

setup_callback_client also return -EINVAL when ses == NULL with conn.cb_xprt == NULL.

Thanks, yes, after looking over this carefully I don't believe we can call setup_callback_client with ses NULL but conn->cb_xprt non-NULL, so this is just a false positive from coverity.

ses and conn->cb_xprt will be set in the same condition before calling setup_callback_client,

996 c = __nfsd4_find_backchannel(clp);
997 if (c) {

 998                 svc_xprt_get(c->cn_xprt);
 999                 conn.cb_xprt = c->cn_xprt;
1000                 ses = c->cn_session;
Quoted 7 lines

1001 }
1002 spin_unlock(&clp->cl_lock);
1003
1004 err = setup_callback_client(clp, &conn, ses);

ses and conn->cb_xprt will be NULL or non-NULL in the same time,
so that, call setup_calback_client with ses NULL but conn->cb_xprt non-NULL will not appear.

Agreed but you also need to check what happens in the case where c is NULL.

This is much more confusing than necessary. But I'm inclined to hold off on any cleanup here while jlayton's patches are still pending.

--b.

Quoted 26 lines

thanks,
Kinglong Mee

Quoted 22 lines
Quoted 21 lines
Quoted 21 lines
fs/nfsd/nfs4callback.c +6 -5
-	if (err) {
-		nfsd4_mark_cb_down(clp, err);
-		return;
-	}
+	if (err)
+		goto out_no_connection;
 	/* Yay, the callback channel's back! Restart any callbacks: */
 	list_for_each_entry(cb, &clp->cl_callbacks, cb_per_client)
 		run_nfsd4_cb(cb);
+	return;
+out_no_connection:
+	nfsd4_mark_cb_down(clp, err);
+	return;
 }
 
 static void nfsd4_do_callback_rpc(struct work_struct *w)
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw
Quoted 44 lines · J. Bruce Fields
Quoted 43 lines · Kinglong Mee
Quoted 21 lines · J. Bruce Fields
Quoted 20 lines · Kinglong Mee
Quoted 10 lines · J. Bruce Fields

The local variable "ses" will be left NULL here in the case we fail to find a connection. Spotted by a coverity scan.

Signed-off-by J. Bruce Fields <[email protected]>
fs/nfsd/nfs4callback.c +0 -0
index 2c73cae9899d..fe22cd5c42d3 100644

fs/nfsd/nfs4callback.c static void nfsd4_process_cb_update(struct nfsd4_callback *cb) · line 1005

Quoted 5 lines
fs/nfsd/nfs4callback.c +2 -0
@@ -1001,14 +1001,18 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
 	}
 	spin_unlock(&clp->cl_lock);
 
+	if (!c)
+		goto out_no_connection;

Setting err to -EINVAL maybe better.
Otherwise, nfsd4_mark_cb_down will be called with err == 0.

Quoted 7 lines
Quoted 3 lines

fs/nfsd/nfs4callback.c static void nfsd4_process_cb_update(struct nfsd4_callback *cb) · line 1006

Quoted 1 line
fs/nfsd/nfs4callback.c +0 -0
 	err = setup_callback_client(clp, &conn, ses);

setup_callback_client also return -EINVAL when ses == NULL with conn.cb_xprt == NULL.

Thanks, yes, after looking over this carefully I don't believe we can call setup_callback_client with ses NULL but conn->cb_xprt non-NULL, so this is just a false positive from coverity.

ses and conn->cb_xprt will be set in the same condition before calling setup_callback_client,

996 c = __nfsd4_find_backchannel(clp);
997 if (c) {

 998                 svc_xprt_get(c->cn_xprt);
 999                 conn.cb_xprt = c->cn_xprt;
1000                 ses = c->cn_session;

1001 }
1002 spin_unlock(&clp->cl_lock);
1003
1004 err = setup_callback_client(clp, &conn, ses);

Quoted 8 lines
Quoted 2 lines

ses and conn->cb_xprt will be NULL or non-NULL in the same time,
so that, call setup_calback_client with ses NULL but conn->cb_xprt non-NULL will not appear.

Agreed but you also need to check what happens in the case where c is NULL.

This is much more confusing than necessary. But I'm inclined to hold off on any cleanup here while jlayton's patches are still pending.

Got it.
I also have some cleanup, will send after jlayton's patches merged.

thanks,
Kinglong Mee

Quoted 30 lines

--b.

Quoted 26 lines

thanks,
Kinglong Mee

Quoted 22 lines
Quoted 21 lines
Quoted 21 lines
fs/nfsd/nfs4callback.c +6 -5
-	if (err) {
-		nfsd4_mark_cb_down(clp, err);
-		return;
-	}
+	if (err)
+		goto out_no_connection;
 	/* Yay, the callback channel's back! Restart any callbacks: */
 	list_for_each_entry(cb, &clp->cl_callbacks, cb_per_client)
 		run_nfsd4_cb(cb);
+	return;
+out_no_connection:
+	nfsd4_mark_cb_down(clp, err);
+	return;
 }
 
 static void nfsd4_do_callback_rpc(struct work_struct *w)
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html