Lustre Klocwork fixes

PATCH Superseded Oleg Drokin 3 participants 13 messages Apr 27, 2014 Staging

lore

Share & tools

lore

Oleg Drokin

author

2014-04-27 21:17 UTC #0 lore raw

cover Lustre Klocwork fixes

This is just splitting big lnet fixes patch from Klocwork static analysis tool into smaller bits.

Dmitry Eremin (5):
staging/lustre/lnet: fix potential null pointer dereference staging/lustre/lnet: remove unused variable staging/lustre/lnet: fix potential null pointer dereference staging/lustre/lnet: fix potential null pointer dereference staging/lustre/lnet: fix potential null pointer dereference

 drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c |  8 ++++++--
 drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c    |  4 ++--
 drivers/staging/lustre/lnet/lnet/api-ni.c              |  6 +++---
 drivers/staging/lustre/lnet/lnet/router.c              |  3 ++-
 drivers/staging/lustre/lnet/selftest/framework.c       | 14 +++++++++++---
 5 files changed, 24 insertions(+), 11 deletions(-)

Signature · 1 line

1.9.0

Oleg Drokin

author

2014-04-27 21:17 UTC #1 lore raw

PATCH 1/5 staging/lustre/lnet: fix potential null pointer dereference

From: Dmitry Eremin <[email protected]>

Null pointer 'best_iface' that comes from line 802 may be dereferenced at line 832.
found by Klocwork Insight tool

Signed-off-by Dmitry Eremin <[email protected]>

Reviewed-on: http://review.whamcloud.com/9386
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-4629

Reviewed-by John L. Hammond <[email protected]>

Reviewed-by Isaac Huang <[email protected]>

Signed-off-by Oleg Drokin <[email protected]>

---
 drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c +2 -2

index 21d36ee..516f623 100644
@@ -829,14 +829,14 @@ ksocknal_select_ips(ksock_peer_t *peer, __u32 *peerips, int n_peerips)
 				best_npeers = iface->ksni_npeers;
 			}
 
+			LASSERT(best_iface != NULL);
+
 			best_iface->ksni_npeers++;
 			ip = best_iface->ksni_ipaddr;
 			peer->ksnp_passive_ips[i] = ip;
 			peer->ksnp_n_passive_ips = i+1;
 		}
 
-		LASSERT (best_iface != NULL);
-
 		/* mark the best matching peer IP used */
 		j = ksocknal_match_peerip(best_iface, peerips, n_peerips);
 		peerips[j] = 0;

Signature · 1 line

1.9.0

One Thousand Gnomes

2014-04-27 22:06 UTC #2 lore raw

Re: [PATCH 1/5] staging/lustre/lnet: fix potential null pointer dereference

Quoted 12 lines

Reviewed-by John L. Hammond <[email protected]>
Reviewed-by Isaac Huang <[email protected]>
Signed-off-by Oleg Drokin <[email protected]>
---
 drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c +0 -0
index 21d36ee..516f623 100644

drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c ksocknal_select_ips(ksock_peer_t *peer, __u32 *peerips, int n_peerips) · line 833

Quoted 5 lines

drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c +2 -0

@@ -829,14 +829,14 @@ ksocknal_select_ips(ksock_peer_t *peer, __u32 *peerips, int n_peerips)
 				best_npeers = iface->ksni_npeers;
 			}
 
+			LASSERT(best_iface != NULL);
+

And this solves the problem how ???

These dont' appear that helpful. If we dereference a NULL point we'll already get a suitably spectacular oops and call trace dump.

Alan

Oleg Drokin

author

2014-04-27 22:20 UTC #3 lore raw

Re: [PATCH 1/5] staging/lustre/lnet: fix potential null pointer dereference

Quoted 15 lines · One Thousand Gnomes

Quoted 15 lines
Reviewed-by John L. Hammond <[email protected]>
Reviewed-by Isaac Huang <[email protected]>
Signed-off-by Oleg Drokin <[email protected]>
---
drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c +0 -0
index 21d36ee..516f623 100644
@@ -829,14 +829,14 @@ ksocknal_select_ips(ksock_peer_t *peer, __u32 *peerips, int n_peerips)
 				best_npeers = iface->ksni_npeers;
 			}
 

Quoted 4 lines

drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c ksocknal_select_ips(ksock_peer_t *peer, __u32 *peerips, int n_peerips) · line 833
Quoted 2 lines
drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c +2 -0
+			LASSERT(best_iface != NULL);
+
And this solves the problem how ???

This does not really solve anything.
Just moves the check to where it's actually going to do anything instead of the check being guarded by the NULL pointer access earlier on. Could have been removed instead of course to just get an oops at all times.

Greg Kroah-Hartman

maintainer

2014-04-27 22:38 UTC #4 lore raw

Re: [PATCH 1/5] staging/lustre/lnet: fix potential null pointer dereference

Quoted 17 lines · Oleg Drokin

Quoted 15 lines · One Thousand Gnomes
Quoted 15 lines
Reviewed-by John L. Hammond <[email protected]>
Reviewed-by Isaac Huang <[email protected]>
Signed-off-by Oleg Drokin <[email protected]>
---
drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c +0 -0
index 21d36ee..516f623 100644
@@ -829,14 +829,14 @@ ksocknal_select_ips(ksock_peer_t *peer, __u32 *peerips, int n_peerips)
 				best_npeers = iface->ksni_npeers;
 			}
 

Quoted 8 lines

Quoted 4 lines
drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c ksocknal_select_ips(ksock_peer_t *peer, __u32 *peerips, int n_peerips) · line 833
Quoted 2 lines
drivers/staging/lustre/lnet/klnds/socklnd/socklnd.c +2 -0
+			LASSERT(best_iface != NULL);
+
And this solves the problem how ???
This does not really solve anything.
Just moves the check to where it's actually going to do anything instead of the check being guarded by the NULL pointer access earlier on. Could have been removed instead of course to just get an oops at all times.

Then please just remove it, or, even better yet, fix it properly if there is a way that this pointer can be NULL.

greg k-h

Oleg Drokin

author

2014-04-27 21:19 UTC #5 lore raw

PATCH 2/5 staging/lustre/lnet: remove unused variable

In reply to Oleg Drokin

From: Dmitry Eremin <[email protected]>

Local variable 'hash' is never used
found by Klocwork Insight tool

Signed-off-by Dmitry Eremin <[email protected]>

Reviewed-on: http://review.whamcloud.com/9386
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-4629

Reviewed-by John L. Hammond <[email protected]>

Reviewed-by Isaac Huang <[email protected]>

Signed-off-by Oleg Drokin <[email protected]>

---
 drivers/staging/lustre/lnet/lnet/api-ni.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/staging/lustre/lnet/lnet/api-ni.c +3 -3

index 85b8d81..3f1fdaa 100644
@@ -127,8 +127,7 @@ lnet_create_remote_nets_table(void)
 static void
 lnet_destroy_remote_nets_table(void)
 {
-	int		i;
-	struct list_head	*hash;
+	int i;
 
 	if (the_lnet.ln_remote_nets_hash == NULL)
 		return;

@@ -137,7 +136,8 @@ lnet_destroy_remote_nets_table(void)
 		LASSERT(list_empty(&the_lnet.ln_remote_nets_hash[i]));
 
 	LIBCFS_FREE(the_lnet.ln_remote_nets_hash,
-		    LNET_REMOTE_NETS_HASH_SIZE * sizeof(*hash));
+		    LNET_REMOTE_NETS_HASH_SIZE *
+		    sizeof(the_lnet.ln_remote_nets_hash[0]));
 	the_lnet.ln_remote_nets_hash = NULL;
 }

Signature · 1 line

1.9.0

Oleg Drokin

author

2014-04-27 21:17 UTC #6 lore raw

PATCH 3/5 staging/lustre/lnet: fix potential null pointer dereference

In reply to Oleg Drokin

From: Dmitry Eremin <[email protected]>

Pointer 'tsc' returned from call to function 'sfw_find_test_case' at line 571 may be NULL and will be dereferenced at line 572. found by Klocwork Insight tool

Signed-off-by Dmitry Eremin <[email protected]>

Reviewed-on: http://review.whamcloud.com/9386
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-4629

Reviewed-by John L. Hammond <[email protected]>

Reviewed-by Isaac Huang <[email protected]>

Signed-off-by Oleg Drokin <[email protected]>

---
 drivers/staging/lustre/lnet/selftest/framework.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

drivers/staging/lustre/lnet/selftest/framework.c +11 -3

index 050723a..c141f93 100644
@@ -547,10 +547,16 @@ sfw_test_rpc_fini (srpc_client_rpc_t *rpc)
 static inline int
 sfw_test_buffers(sfw_test_instance_t *tsi)
 {
-	struct sfw_test_case	*tsc = sfw_find_test_case(tsi->tsi_service);
-	struct srpc_service	*svc = tsc->tsc_srv_service;
+	struct sfw_test_case	*tsc;
+	struct srpc_service	*svc;
 	int			nbuf;
 
+	LASSERT(tsi != NULL);
+	tsc = sfw_find_test_case(tsi->tsi_service);
+	LASSERT(tsc != NULL);
+	svc = tsc->tsc_srv_service;
+	LASSERT(svc != NULL);
+
 	nbuf = min(svc->sv_wi_total, tsi->tsi_loop) / svc->sv_ncpts;
 	return max(SFW_TEST_WI_MIN, nbuf + SFW_TEST_WI_EXTRA);
 }

@@ -595,8 +601,10 @@ sfw_load_test(struct sfw_test_instance *tsi)
 void
 sfw_unload_test(struct sfw_test_instance *tsi)
 {
-	struct sfw_test_case *tsc = sfw_find_test_case(tsi->tsi_service);
+	struct sfw_test_case *tsc;
 
+	LASSERT(tsi != NULL);
+	tsc = sfw_find_test_case(tsi->tsi_service);
 	LASSERT(tsc != NULL);
 
 	if (tsi->tsi_is_client)

Signature · 1 line

1.9.0

Greg Kroah-Hartman

maintainer

2014-04-27 22:39 UTC #7 lore raw

Re: [PATCH 3/5] staging/lustre/lnet: fix potential null pointer dereference

Quoted 21 lines · Oleg Drokin

From: Dmitry Eremin <[email protected]>
Pointer 'tsc' returned from call to function 'sfw_find_test_case' at line 571 may be NULL and will be dereferenced at line 572. found by Klocwork Insight tool
Signed-off-by Dmitry Eremin <[email protected]>
Reviewed-on: http://review.whamcloud.com/9386
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-4629
Reviewed-by John L. Hammond <[email protected]>
Reviewed-by Isaac Huang <[email protected]>
Signed-off-by Oleg Drokin <[email protected]>
---
 drivers/staging/lustre/lnet/selftest/framework.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
drivers/staging/lustre/lnet/selftest/framework.c +0 -0
index 050723a..c141f93 100644

drivers/staging/lustre/lnet/selftest/framework.c sfw_test_rpc_fini (srpc_client_rpc_t *rpc) · line 554

Quoted 10 lines

drivers/staging/lustre/lnet/selftest/framework.c +3 -2

@@ -547,10 +547,16 @@ sfw_test_rpc_fini (srpc_client_rpc_t *rpc)
 static inline int
 sfw_test_buffers(sfw_test_instance_t *tsi)
 {
-	struct sfw_test_case	*tsc = sfw_find_test_case(tsi->tsi_service);
-	struct srpc_service	*svc = tsc->tsc_srv_service;
+	struct sfw_test_case	*tsc;
+	struct srpc_service	*svc;
 	int			nbuf;
 
+	LASSERT(tsi != NULL);

Asserts suck rocks, which is why we avoid them at all costs. Please just fix the code to work properly. If that parameter can be NULL, then properly handle the error and return, don't oops the kernel.

greg k-h

Oleg Drokin

author

2014-04-27 21:18 UTC #8 lore raw

PATCH 4/5 staging/lustre/lnet: fix potential null pointer dereference

In reply to Oleg Drokin

From: Dmitry Eremin <[email protected]>

Null pointer 'cp' that comes from line 2544 may be dereferenced at line 2618.
found by Klocwork Insight tool

Signed-off-by Dmitry Eremin <[email protected]>

Reviewed-on: http://review.whamcloud.com/9386
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-4629

Reviewed-by John L. Hammond <[email protected]>

Reviewed-by Isaac Huang <[email protected]>

Signed-off-by Oleg Drokin <[email protected]>

---
 drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd_cb.c +6 -2

index 6173e74..9bf6c94 100644
@@ -2609,13 +2609,17 @@ kiblnd_rejected (kib_conn_t *conn, int reason, void *priv, int priv_nob)
 
 			case IBLND_REJECT_MSG_QUEUE_SIZE:
 				CERROR("%s rejected: incompatible message queue depth %d, %d\n",
-				       libcfs_nid2str(peer->ibp_nid), cp->ibcp_queue_depth,
+				       libcfs_nid2str(peer->ibp_nid),
+				       cp != NULL ? cp->ibcp_queue_depth :
+				       IBLND_MSG_QUEUE_SIZE(rej->ibr_version),
 				       IBLND_MSG_QUEUE_SIZE(conn->ibc_version));
 				break;
 
 			case IBLND_REJECT_RDMA_FRAGS:
 				CERROR("%s rejected: incompatible # of RDMA fragments %d, %d\n",
-				       libcfs_nid2str(peer->ibp_nid), cp->ibcp_max_frags,
+				       libcfs_nid2str(peer->ibp_nid),
+				       cp != NULL ? cp->ibcp_max_frags :
+				       IBLND_RDMA_FRAGS(rej->ibr_version),
 				       IBLND_RDMA_FRAGS(conn->ibc_version));
 				break;

Signature · 1 line

1.9.0

Oleg Drokin

author

2014-04-27 21:17 UTC #9 lore raw

PATCH 5/5 staging/lustre/lnet: fix potential null pointer dereference

In reply to Oleg Drokin

From: Dmitry Eremin <[email protected]>

Pointer 'ni' checked for NULL at line 1569 may be passed to function and may be dereferenced there by passing argument 1 to function 'lnet_ni_notify_locked' at line 1621.
found by Klocwork Insight tool

Signed-off-by Dmitry Eremin <[email protected]>

Reviewed-on: http://review.whamcloud.com/9386
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-4629

Reviewed-by John L. Hammond <[email protected]>

Reviewed-by Isaac Huang <[email protected]>

Signed-off-by Oleg Drokin <[email protected]>

---
 drivers/staging/lustre/lnet/lnet/router.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/staging/lustre/lnet/lnet/router.c +2 -1

index 995f509..ba0278e 100644
@@ -1559,7 +1559,8 @@ lnet_notify(lnet_ni_t *ni, lnet_nid_t nid, int alive, cfs_time_t when)
 
 	lnet_notify_locked(lp, ni == NULL, alive, when);
 
-	lnet_ni_notify_locked(ni, lp);
+	if (ni != NULL)
+		lnet_ni_notify_locked(ni, lp);
 
 	lnet_peer_decref_locked(lp);

Signature · 1 line

1.9.0

Greg Kroah-Hartman

maintainer

2014-04-27 22:38 UTC #10 lore raw

Re: [PATCH 5/5] staging/lustre/lnet: fix potential null pointer dereference

Quoted 22 lines · Oleg Drokin

From: Dmitry Eremin <[email protected]>
Pointer 'ni' checked for NULL at line 1569 may be passed to function and may be dereferenced there by passing argument 1 to function 'lnet_ni_notify_locked' at line 1621.
found by Klocwork Insight tool
Signed-off-by Dmitry Eremin <[email protected]>
Reviewed-on: http://review.whamcloud.com/9386
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-4629
Reviewed-by John L. Hammond <[email protected]>
Reviewed-by Isaac Huang <[email protected]>
Signed-off-by Oleg Drokin <[email protected]>
---
 drivers/staging/lustre/lnet/lnet/router.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/staging/lustre/lnet/lnet/router.c +0 -0
index 995f509..ba0278e 100644

drivers/staging/lustre/lnet/lnet/router.c lnet_notify(lnet_ni_t *ni, lnet_nid_t nid, int alive, cfs_time_t when) · line 1563

Quoted 6 lines

drivers/staging/lustre/lnet/lnet/router.c +2 -1

@@ -1559,7 +1559,8 @@ lnet_notify(lnet_ni_t *ni, lnet_nid_t nid, int alive, cfs_time_t when)
 
 	lnet_notify_locked(lp, ni == NULL, alive, when);
 
-	lnet_ni_notify_locked(ni, lp);
+	if (ni != NULL)
+		lnet_ni_notify_locked(ni, lp);

Why can't lnet_ni_notify_locked() accept NULL as an input?

Oleg Drokin

author

2014-04-27 23:28 UTC #11 lore raw

Re: [PATCH 5/5] staging/lustre/lnet: fix potential null pointer dereference

Hello!

Quoted 6 lines · Greg Kroah-Hartman

drivers/staging/lustre/lnet/lnet/router.c lnet_notify(lnet_ni_t *ni, lnet_nid_t nid, int alive, cfs_time_t when) · line 1563
Quoted 4 lines
drivers/staging/lustre/lnet/lnet/router.c +2 -1
-	lnet_ni_notify_locked(ni, lp);
+	if (ni != NULL)
+		lnet_ni_notify_locked(ni, lp);
Why can't lnet_ni_notify_locked() accept NULL as an input?

It makes no sense, because then there is nowhere to send the notification.
That said, it appears a race is possible where one caller updated let_peer structure to ask for a notification and then we fell through here with a NULL ni and called into lnet_ni_notify_locked
where we'd try to dereference this NULL ni.
But this is the only called that accepts separate ni and lp, where as the only other caller gets them from the same struct where they are updated more in sync.

I guess it makes sense to update lnet_ni_notify_locked as a future-proofing excercise.

Thanks, I'll update this patch.

Bye,
Oleg

Greg Kroah-Hartman

maintainer

2014-04-27 22:36 UTC #12 lore raw

In reply to Oleg Drokin

Quoted 9 lines · Oleg Drokin

This is just splitting big lnet fixes patch from Klocwork static analysis tool into smaller bits.
Dmitry Eremin (5):
staging/lustre/lnet: fix potential null pointer dereference staging/lustre/lnet: remove unused variable staging/lustre/lnet: fix potential null pointer dereference staging/lustre/lnet: fix potential null pointer dereference staging/lustre/lnet: fix potential null pointer dereference

You can't send the same subject: for all patches, that's not good. Please be more specific.

greg k-h