Trivial fixes
cover Trivial fixes
Hi Doug,
I'm sending to you small collection of trivial fixes to core code.
It fixes forgotten skb release call, potential overrun in CMA. SA and two code simplifications which doesn't introduce any code change behavior.
Available in the "topic/fix-core" topic branch of this git repo:
git://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git
Or for browsing:
https://git.kernel.org/cgit/linux/kernel/git/leon/linux-rdma.git/log/?h=topic/fix-core
Mark Bloch (4):
IB/iwpm: Fix a potential skb leak
IB/core: Remove unnecessary check in ibnl_rcv_msg IB/core: Fix a potential array overrun in CMA and SA agent IB/sa: Use nlmsg_free instead of kfree_skb
drivers/infiniband/core/cma.c | 2 +- drivers/infiniband/core/iwcm.c | 2 +- drivers/infiniband/core/iwpm_util.c | 1 + drivers/infiniband/core/netlink.c | 3 +-- drivers/infiniband/core/sa_query.c | 4 ++-- 5 files changed, 6 insertions(+), 6 deletions(-)
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1462340165-16067-1-git-send-email-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
PATCH 1/4 IB/iwpm: Fix a potential skb leak
From: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/[email protected]>
In case ibnl_put_msg fails in send_nlmsg_done, the function returns with -ENOMEM without freeing.
This patch fixes this behavior.
--- drivers/infiniband/core/iwpm_util.c | 1 + 1 file changed, 1 insertion(+)
drivers/infiniband/core/iwpm_util.c +1 -0
@@ -634,6 +634,7 @@ static int send_nlmsg_done(struct sk_buff *skb, u8 nl_client, int iwpm_pid)if (!(ibnl_put_msg (skb, &nlh,0 ,0 , nl_client,RDMA_NL_IWPM_MAPINFO ,NLM_F_MULTI ))) {pr_warn ("%s Unable to put NLMSG_DONE\n" , __func__); +dev_kfree_skb (skb);return -ENOMEM ; } nlh->nlmsg_type =NLMSG_DONE ;
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1462340165-16067-2-git-send-email-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Steve Wise
Reviewed-byRE: [PATCH rdma-next 1/4] IB/iwpm: Fix a potential skb leak
Quoted 39 lines
-----Original Message-----
From: [email protected] [mailto:linux-rdma- [email protected]] On Behalf Of Leon Romanovsky
Sent: Wednesday, May 04, 2016 12:36 AM
To: [email protected]
Cc: [email protected]; markb-VPRAkNaXOzVWk0Htik3J/[email protected]; majd-VPRAkNaXOzVWk0Htik3J/[email protected]; matanb-VPRAkNaXOzVWk0Htik3J/[email protected]; Leon Romanovsky
Subject: [PATCH rdma-next 1/4] IB/iwpm: Fix a potential skb leakFrom: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/[email protected]>
In case
ibnl_put_msgfails insend_nlmsg_done, the function returns with-ENOMEMwithout freeing.This patch fixes this behavior.
Fixes 30dc5e63d6a5 ("RDMA/core: Add support for iWARP Port Mapper user spaceservice")
Signed-off-by Leon Romanovsky <[email protected]>--- drivers/infiniband/core/iwpm_util.c | 1 + 1 file changed, 1 insertion(+)drivers/infiniband/core/iwpm_util.c +1
-0@@ -634,6 +634,7 @@ static int send_nlmsg_done(struct sk_buff *skb, u8 nl_client, int iwpm_pid)if (!(ibnl_put_msg (skb, &nlh,0 ,0 , nl_client,RDMA_NL_IWPM_MAPINFO ,NLM_F_MULTI ))) {pr_warn ("%s Unable to put NLMSG_DONE\n" , __func__); +dev_kfree_skb (skb);return -ENOMEM ; }
Quoted 3 lines
nlh->nlmsg_type=NLMSG_DONE;
--
2.1.4
Looks good.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 2/4 IB/core: Remove unnecessary check in ibnl_rcv_msg
In reply to a message not in the archiveFrom: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/[email protected]>
RDMA_NL_GET_OP is defined like this: (type & ((1 << 10) - 1)) which means op (defined as an int) can never be a negative number.
--- drivers/infiniband/core/netlink.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/infiniband/core/netlink.c +1 -2
@@ -155,8 +155,7 @@ static int ibnl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)list_for_each_entry (client, &client_list, list) {if (client->index == index) { -if (op <0 || op >= client->nops || - !client->cb_table[op].dump) +if (op >= client->nops || !client->cb_table[op].dump)return -EINVAL ;/*
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1462340165-16067-3-git-send-email-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
RE: [PATCH rdma-next 2/4] IB/core: Remove unnecessary check in ibnl_rcv_msg
Quoted 17 lines
-----Original Message-----
From: [email protected] [mailto:linux-rdma- [email protected]] On Behalf Of Leon Romanovsky
Sent: Wednesday, May 04, 2016 12:36 AM
To: [email protected]
Cc: [email protected]; markb-VPRAkNaXOzVWk0Htik3J/[email protected]; majd-VPRAkNaXOzVWk0Htik3J/[email protected]; matanb-VPRAkNaXOzVWk0Htik3J/[email protected]; Leon Romanovsky
Subject: [PATCH rdma-next 2/4] IB/core: Remove unnecessary check inibnl_rcv_msgFrom: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/[email protected]>
RDMA_NL_GET_OPis defined like this: (type & ((1 << 10) - 1)) which means op (defined as an int) can never be a negative number.
Review
Quoted 8 lines
Fixes b2cbae2c2487 ('RDMA: Add netlink infrastructure')Signed-off-by Leon Romanovsky <[email protected]>--- drivers/infiniband/core/netlink.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)(quoted diff) +0
-0
b/drivers/infiniband/core/netlink.c
drivers/infiniband/core/netlink.c static int ibnl_rcv_msg(struct sk_buff *skb, struct
Quoted 4 lines
index d47df93..aa491f7 100644
drivers/infiniband/core/netlink.c +0
-0@@ -155,8 +155,7 @@ static int ibnl_rcv_msg(struct sk_buff *skb, struct
nlmsghdr
Quoted 10 lines
drivers/infiniband/core/netlink.c +1
-2*nlh)list_for_each_entry (client, &client_list, list) {if (client->index == index) { -if (op <0 || op >= client->nops || - !client->cb_table[op].dump) +if (op >= client->nops || !client->cb_table[op].dump)return -EINVAL ;/*
Should op be made unsigned as well? That would make it clearer.
Steve.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH rdma-next 2/4] IB/core: Remove unnecessary check in ibnl_rcv_msg
Quoted 31 lines · Steve Wise
Quoted 8 lines
From: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/[email protected]>
RDMA_NL_GET_OPis defined like this: (type & ((1 << 10) - 1)) which means op (defined as an int) can never be a negative number.Review
Quoted 8 lines
Fixes b2cbae2c2487 ('RDMA: Add netlink infrastructure')Signed-off-by Leon Romanovsky <[email protected]>--- drivers/infiniband/core/netlink.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)(quoted diff) +0
-0
b/drivers/infiniband/core/netlink.cdrivers/infiniband/core/netlink.c static int ibnl_rcv_msg(struct sk_buff *skb, struct
Quoted 4 lines
index d47df93..aa491f7 100644
drivers/infiniband/core/netlink.c +0
-0@@ -155,8 +155,7 @@ static int ibnl_rcv_msg(struct sk_buff *skb, structnlmsghdr
Quoted 9 lines
drivers/infiniband/core/netlink.c +1
-2*nlh)list_for_each_entry (client, &client_list, list) {if (client->index == index) { -if (op <0 || op >= client->nops || - !client->cb_table[op].dump) +if (op >= client->nops || !client->cb_table[op].dump)return -EINVAL ;
Quoted 3 lines
Quoted 1 line
/*
Should op be made unsigned as well? That would make it clearer.
I was going to make the same comment.
Ira
Quoted 8 lines
Steve.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 3/4 IB/core: Fix a potential array overrun in CMA and SA agent
In reply to a message not in the archiveFrom: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/[email protected]>
Fix array overrun when going over callback table.
In declaration of callback table, the max size isn't provided and in registration phase, it is provided.
There is potential scenario where a new operation is added and it is not supported by current client. The acceptance of such operation by ib_netlink will cause to array overrun.
--- drivers/infiniband/core/cma.c | 2 +- drivers/infiniband/core/iwcm.c | 2 +- drivers/infiniband/core/sa_query.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)
drivers/infiniband/core/cma.c +1 -1
@@ -4240,7 +4240,7 @@ out:return skb->len; } -static const struct ibnl_client_cbs cma_cb_table[] = { +static const struct ibnl_client_cbs cma_cb_table[RDMA_NL_RDMA_CM_NUM_OPS ] = { [RDMA_NL_RDMA_CM_ID_STATS ] = { .dump = cma_get_id_stats, .module =THIS_MODULE }, };
drivers/infiniband/core/iwcm.c +1 -1
@@ -59,7 +59,7 @@ MODULE_AUTHOR("Tom Tucker");MODULE_DESCRIPTION ("iWARP CM" );MODULE_LICENSE ("Dual BSD/GPL" ); -static struct ibnl_client_cbs iwcm_nl_cb_table[] = { +static struct ibnl_client_cbs iwcm_nl_cb_table[RDMA_NL_IWPM_NUM_OPS ] = { [RDMA_NL_IWPM_REG_PID ] = {.dump = iwpm_register_pid_cb}, [RDMA_NL_IWPM_ADD_MAPPING ] = {.dump = iwpm_add_mapping_cb}, [RDMA_NL_IWPM_QUERY_MAPPING ] = {.dump = iwpm_add_and_query_mapping_cb},
drivers/infiniband/core/sa_query.c +1 -1
@@ -838,7 +838,7 @@ resp_out:return skb->len; } -static struct ibnl_client_cbs ib_sa_cb_table[] = { +static struct ibnl_client_cbs ib_sa_cb_table[RDMA_NL_LS_NUM_OPS ] = { [RDMA_NL_LS_OP_RESOLVE ] = { .dump = ib_nl_handle_resolve_resp, .module =THIS_MODULE },
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1462340165-16067-4-git-send-email-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Steve Wise
Reviewed-byRE: [PATCH rdma-next 3/4] IB/core: Fix a potential array overrun in CMA and SA agent
Quoted 10 lines
-----Original Message-----
From: [email protected] [mailto:linux-rdma- [email protected]] On Behalf Of Leon Romanovsky
Sent: Wednesday, May 04, 2016 12:36 AM
To: [email protected]
Cc: [email protected]; markb-VPRAkNaXOzVWk0Htik3J/[email protected]; majd-VPRAkNaXOzVWk0Htik3J/[email protected]; matanb-VPRAkNaXOzVWk0Htik3J/[email protected]; Leon Romanovsky
Subject: [PATCH rdma-next 3/4] IB/core: Fix a potential array overrun in CMA
and
Quoted 51 lines
SA agent
From: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/[email protected]>
Fix array overrun when going over callback table.
In declaration of callback table, the max size isn't provided and in registration phase, it is provided.There is potential scenario where a new operation is added and it is not supported by current client. The acceptance of such operation by
ib_netlinkwill cause to array overrun.Fixes 809d5fc9bf65 ("infiniband: pass rdma_cm module to netlink_dump_start")Fixes b493d91d333e ("iwcm: common code for port mapper")Fixes 2ca546b92a02 ("IB/sa: Route SA pathrecord query through netlink")Signed-off-by Leon Romanovsky <[email protected]>--- drivers/infiniband/core/cma.c | 2 +- drivers/infiniband/core/iwcm.c | 2 +- drivers/infiniband/core/sa_query.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)drivers/infiniband/core/cma.c +1
-1@@ -4240,7 +4240,7 @@ out:return skb->len; } -static const struct ibnl_client_cbs cma_cb_table[] = { +static const struct ibnl_client_cbs cma_cb_table[RDMA_NL_RDMA_CM_NUM_OPS ] = { [RDMA_NL_RDMA_CM_ID_STATS ] = { .dump = cma_get_id_stats, .module =THIS_MODULE },};
drivers/infiniband/core/iwcm.c +1
-1@@ -59,7 +59,7 @@ MODULE_AUTHOR("Tom Tucker");MODULE_DESCRIPTION ("iWARP CM" );MODULE_LICENSE ("Dual BSD/GPL" ); -static struct ibnl_client_cbs iwcm_nl_cb_table[] = { +static struct ibnl_client_cbs iwcm_nl_cb_table[RDMA_NL_IWPM_NUM_OPS ] = { [RDMA_NL_IWPM_REG_PID ] = {.dump = iwpm_register_pid_cb}, [RDMA_NL_IWPM_ADD_MAPPING ] = {.dump = iwpm_add_mapping_cb}, [RDMA_NL_IWPM_QUERY_MAPPING] = {.dump =
iwpm_add_and_query_mapping_cb},
Review
Quoted 1 line
(quoted diff) +0
-0
b/drivers/infiniband/core/sa_query.c
drivers/infiniband/core/sa_query.c resp_out: · line 844
Quoted 12 lines
index 8a09c0f..24c2aac 100644
drivers/infiniband/core/sa_query.c +1
-1@@ -838,7 +838,7 @@ resp_out:return skb->len; } -static struct ibnl_client_cbs ib_sa_cb_table[] = { +static struct ibnl_client_cbs ib_sa_cb_table[RDMA_NL_LS_NUM_OPS ] = { [RDMA_NL_LS_OP_RESOLVE ] = { .dump = ib_nl_handle_resolve_resp, .module =THIS_MODULE },
Looks ok.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH rdma-next 3/4] IB/core: Fix a potential array overrun in CMA and SA agent
In reply to a message not in the archivedrivers/infiniband/core/cma.c out: · line 4245
Quoted 4 lines · Leon Romanovsky
drivers/infiniband/core/cma.c +1
-1-static const struct ibnl_client_cbs cma_cb_table[] = { +static const struct ibnl_client_cbs cma_cb_table[RDMA_NL_RDMA_CM_NUM_OPS ] = { [RDMA_NL_RDMA_CM_ID_STATS ] = { .dump = cma_get_id_stats, .module =THIS_MODULE },
Isn't it much better to change this:
if (ibnl_add_client(RDMA_NL_RDMA_CM, RDMA_NL_RDMA_CM_NUM_OPS, cma_cb_table))
to
if (ibnl_add_client(RDMA_NL_RDMA_CM, NELEMS(cma_cb_table), cma_cb_table))
And eliminate RDMA_NL_RDMA_CM_NUM_OPS entirely?
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 20160504181922.GA20554-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org
Re: [PATCH rdma-next 3/4] IB/core: Fix a potential array overrun in CMA and SA agent
Quoted 9 lines · Jason Gunthorpe
drivers/infiniband/core/cma.c out: · line 4245
Quoted 4 lines · Leon Romanovsky
drivers/infiniband/core/cma.c +1
-1-static const struct ibnl_client_cbs cma_cb_table[] = { +static const struct ibnl_client_cbs cma_cb_table[RDMA_NL_RDMA_CM_NUM_OPS ] = { [RDMA_NL_RDMA_CM_ID_STATS ] = { .dump = cma_get_id_stats, .module =THIS_MODULE },Isn't it much better to change this:
Quoted 6 lines
if (
ibnl_add_client(RDMA_NL_RDMA_CM, RDMA_NL_RDMA_CM_NUM_OPS, cma_cb_table))to
if (ibnl_add_client(RDMA_NL_RDMA_CM, NELEMS(cma_cb_table), cma_cb_table))And eliminate
RDMA_NL_RDMA_CM_NUM_OPSentirely?
Well based on their other series I think we may need to revisit the design pattern of netlink altogether. But yea if we just want to fix this we can do this.
Ira
Quoted 6 lines
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 4/4 IB/sa: Use nlmsg_free instead of kfree_skb
In reply to a message not in the archiveFrom: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/[email protected]>
Fixes a direct call to kfree_skb when nlmsg_free should be used.
--- drivers/infiniband/core/sa_query.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/core/sa_query.c +1 -1
@@ -536,7 +536,7 @@ static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask) data =ibnl_put_msg (skb, &nlh, query->seq,0 ,RDMA_NL_LS ,RDMA_NL_LS_OP_RESOLVE ,NLM_F_REQUEST );if (!data) { -kfree_skb (skb); +nlmsg_free (skb);return -EMSGSIZE ; }
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1462340165-16067-5-git-send-email-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Steve Wise
Reviewed-byRE: [PATCH rdma-next 4/4] IB/sa: Use nlmsg_free instead of kfree_skb
Quoted 15 lines
-----Original Message-----
From: [email protected] [mailto:linux-rdma- [email protected]] On Behalf Of Leon Romanovsky
Sent: Wednesday, May 04, 2016 12:36 AM
To: [email protected]
Cc: [email protected]; markb-VPRAkNaXOzVWk0Htik3J/[email protected]; majd-VPRAkNaXOzVWk0Htik3J/[email protected]; matanb-VPRAkNaXOzVWk0Htik3J/[email protected]; Leon Romanovsky
Subject: [PATCH rdma-next 4/4] IB/sa: Usenlmsg_freeinstead ofkfree_skbFrom: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/[email protected]>
Fixes a direct call to
kfree_skbwhennlmsg_freeshould be used.
Review
Quoted 8 lines
Fixes 2ca546b92a02 ('IB/sa: Route SA pathrecord query through netlink')Signed-off-by Leon Romanovsky <[email protected]>--- drivers/infiniband/core/sa_query.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)(quoted diff) +0
-0
b/drivers/infiniband/core/sa_query.c
drivers/infiniband/core/sa_query.c static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t · line 542
Quoted 13 lines
index 24c2aac..5050c61 100644
drivers/infiniband/core/sa_query.c +1
-1@@ -536,7 +536,7 @@ static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask) data =ibnl_put_msg (skb, &nlh, query->seq,0 ,RDMA_NL_LS ,RDMA_NL_LS_OP_RESOLVE ,NLM_F_REQUEST );if (!data) { -kfree_skb (skb); +nlmsg_free (skb);return -EMSGSIZE ; }
Looks good.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Ira Weiny
Reviewed-byRe: [PATCH rdma-next 4/4] IB/sa: Use nlmsg_free instead of kfree_skb
In reply to a message not in the archiveQuoted 7 lines · Leon Romanovsky
From: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/[email protected]>
Fixes a direct call to
kfree_skbwhennlmsg_freeshould be used.Fixes 2ca546b92a02 ('IB/sa: Route SA pathrecord query through netlink')Signed-off-by Leon Romanovsky <[email protected]>
Quoted 24 lines
--- drivers/infiniband/core/sa_query.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/infiniband/core/sa_query.c +1
-1@@ -536,7 +536,7 @@ static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask) data =ibnl_put_msg (skb, &nlh, query->seq,0 ,RDMA_NL_LS ,RDMA_NL_LS_OP_RESOLVE ,NLM_F_REQUEST );if (!data) { -kfree_skb (skb); +nlmsg_free (skb);return -EMSGSIZE ; }Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 1 line · Leon Romanovsky
I'm sending to you small collection of trivial fixes to core code.
Thanks for the feedback.
I'll repost it.