RDMA fixes and refactoring
Subsystems (2)
cover RDMA fixes and refactoring
Changelog:
v0 -> v1:
* Squashed patches related to __ib_device_get_by_index and removed that function from core_priv.h.
* Change write semaphore to be read semaphore in ib_device_get_by_index()
---------------------------------------
Small series of various fixes to netlink, core and RXE found during resource tracking development.
The patches are available in the git repository at:
git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git tags/rdma-next-2018-01-01-1
Thanks
---------------------------------------
Leon Romanovsky (7):
RDMA/rxe: Remove useless export symbol
RDMA/netlink: Simplify code of autoload modules RDMA/core: Replace open-coded variant of put_device RDMA/nldev: Refactor nldev handle to be common function RDMA/core: Provide locked variant of device name to index function RDMA/netlink: Protect device and port queries from device removal RDMA/cma: Mark end of CMA ID messages
drivers/infiniband/core/cma.c | 1 + drivers/infiniband/core/core_priv.h | 2 +- drivers/infiniband/core/device.c | 23 ++++++++++- drivers/infiniband/core/netlink.c | 8 ++-- drivers/infiniband/core/nldev.c | 82 +++++++++++++++++++++++-------------- drivers/infiniband/sw/rxe/rxe.c | 6 --- drivers/infiniband/sw/rxe/rxe.h | 6 ++- 7 files changed, 83 insertions(+), 45 deletions(-)
--
2.15.1
--
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): 20180101110717.29686-1-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
PATCH 1/7 RDMA/rxe: Remove useless export symbol
From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
The RXE driver is standalone module and hence doesn't need to export symbols, move this one liner to be in header file and avoid function implementation in .c file and extra exported symbol.
--- drivers/infiniband/sw/rxe/rxe.c | 6 ------ drivers/infiniband/sw/rxe/rxe.h | 6 +++++- 2 files changed, 5 insertions(+), 7 deletions(-)
drivers/infiniband/sw/rxe/rxe.c +0 -6
@@ -77,12 +77,6 @@ void rxe_release(struct kref *kref)ib_dealloc_device (&rxe->ib_dev); } -void rxe_dev_put (struct rxe_dev *rxe) -{ -kref_put (&rxe->ref_cnt, rxe_release); -} -EXPORT_SYMBOL_GPL (rxe_dev_put); -/* initialize rxe device parameters */ static int rxe_init_device_param (struct rxe_dev *rxe) {
drivers/infiniband/sw/rxe/rxe.h +5 -1
@@ -57,6 +57,7 @@#include "rxe_hdr.h" #include "rxe_param.h" #include "rxe_verbs.h" +#include "rxe_loc.h" #define RXE_UVERBS_ABI_VERSION (1)
@@ -95,7 +96,10 @@ void rxe_remove_all(void);int rxe_rcv (struct sk_buff *skb); -void rxe_dev_put (struct rxe_dev *rxe); +static inline void rxe_dev_put (struct rxe_dev *rxe) +{ +kref_put (&rxe->ref_cnt, rxe_release); +}struct rxe_dev *net_to_rxe (struct net_device *ndev);struct rxe_dev *get_rxe_by_name (const char *name);
Signature · 6 lines
2.15.1 -- 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/7 RDMA/netlink: Simplify code of autoload modules
In reply to a message not in the archiveFrom: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
The request_module() call is internally wrapped by CONFIG_MODULE, so there is no need to check it in our RDMA code too.
Refactor the code to simplify the code.
--- drivers/infiniband/core/netlink.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/infiniband/core/netlink.c +3 -5
@@ -81,15 +81,13 @@ static bool is_nl_valid(unsigned int type, unsigned int op)if (!is_nl_msg_valid (type, op))return false; - cb_table = rdma_nl_types[type].cb_table; -#ifdef CONFIG_MODULES -if (!cb_table) { +if (!rdma_nl_types[type].cb_table) {mutex_unlock (&rdma_nl_mutex);request_module ("rdma-netlink-subsys-%d" , type);mutex_lock (&rdma_nl_mutex); - cb_table = rdma_nl_types[type].cb_table; } -#endif + + cb_table = rdma_nl_types[type].cb_table;if (!cb_table || (!cb_table[op].dump && !cb_table[op].doit))return false;
Signature · 6 lines
2.15.1 -- 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/7 RDMA/core: Replace open-coded variant of put_device
In reply to a message not in the archiveFrom: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
There is an existing function to decrease reference counter of the device, let's use it.
--- drivers/infiniband/core/device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/core/device.c +1 -1
@@ -272,7 +272,7 @@ void ib_dealloc_device(struct ib_device *device) {WARN_ON (device->reg_state !=IB_DEV_UNREGISTERED && device->reg_state !=IB_DEV_UNINITIALIZED ); -kobject_put (&device->dev.kobj); +put_device (&device->dev); }EXPORT_SYMBOL (ib_dealloc_device);
Signature · 6 lines
2.15.1 -- 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/7 RDMA/nldev: Refactor nldev handle to be common function
In reply to a message not in the archiveFrom: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
The NLDEV commands are using IB device indexes and names as a handler for netlink communications. Put all relevant code into one function, so it will be reused easily.
--- drivers/infiniband/core/nldev.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
drivers/infiniband/core/nldev.c +14 -6
@@ -54,14 +54,23 @@ static const struct nla_policy nldev_policy[RDMA_NLDEV_ATTR_MAX] = { [RDMA_NLDEV_ATTR_DEV_NODE_TYPE] = { .type = NLA_U8 }, }; -static int fill_dev_info (struct sk_buff *msg,struct ib_device *device) +static int fill_nldev_handle (struct sk_buff *msg,struct ib_device *device) { -char fw[IB_FW_VERSION_NAME_MAX ]; -if (nla_put_u32 (msg,RDMA_NLDEV_ATTR_DEV_INDEX , device->index))return -EMSGSIZE ;if (nla_put_string (msg,RDMA_NLDEV_ATTR_DEV_NAME , device->name))return -EMSGSIZE ; + +return 0 ; +} + +static int fill_dev_info (struct sk_buff *msg,struct ib_device *device) +{ +char fw[IB_FW_VERSION_NAME_MAX ]; + +if (fill_nldev_handle (msg, device)) +return -EMSGSIZE ; +if (nla_put_u32 (msg,RDMA_NLDEV_ATTR_PORT_INDEX ,rdma_end_port (device)))return -EMSGSIZE ;
@@ -92,10 +101,9 @@ static int fill_port_info(struct sk_buff *msg,struct ib_port_attr attr;int ret; -if (nla_put_u32 (msg,RDMA_NLDEV_ATTR_DEV_INDEX , device->index)) -return -EMSGSIZE ; -if (nla_put_string (msg,RDMA_NLDEV_ATTR_DEV_NAME , device->name)) +if (fill_nldev_handle (msg, device))return -EMSGSIZE ; +if (nla_put_u32 (msg,RDMA_NLDEV_ATTR_PORT_INDEX , port))return -EMSGSIZE ;
Signature · 6 lines
2.15.1 -- 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 5/7 RDMA/core: Provide locked variant of device name to index function
In reply to a message not in the archiveFrom: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
Add self-contained with locks device name to index function.
--- drivers/infiniband/core/core_priv.h | 1 + drivers/infiniband/core/device.c | 16 ++++++++++++++++ 2 files changed, 17 insertions(+)
drivers/infiniband/core/core_priv.h +1 -0
@@ -295,6 +295,7 @@ static inline int ib_mad_enforce_security(struct ib_mad_agent_private *map,#endif struct ib_device *__ib_device_get_by_index (u32 ifindex); +struct ib_device *ib_device_get_by_index (u32 ifindex);/* RDMA device netlink */ void nldev_init (void );void nldev_exit (void );
drivers/infiniband/core/device.c +16 -0
@@ -145,6 +145,22 @@ struct ib_device *__ib_device_get_by_index(u32 index)return NULL ; } +/* +* Caller is responsible to return refrerence count by calling put_device() +*/ +struct ib_device *ib_device_get_by_index (u32 index) +{ +struct ib_device *device; + +down_read (&lists_rwsem); + device =__ib_device_get_by_index (index); +if (device) +get_device (&device->dev); + +up_read (&lists_rwsem); +return device; +} +static struct ib_device *__ib_device_get_by_name (const char *name) {struct ib_device *device;
Signature · 6 lines
2.15.1 -- 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 6/7 RDMA/netlink: Protect device and port queries from device removal
In reply to a message not in the archiveFrom: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
There is a chance that device will be removed during device query operations and it will cause to kernel panic in the flows which doesn't hold lists_rwsem semaphore.
--- drivers/infiniband/core/core_priv.h | 1 - drivers/infiniband/core/device.c | 5 ++- drivers/infiniband/core/nldev.c | 62 +++++++++++++++++++++++-------------- 3 files changed, 42 insertions(+), 26 deletions(-)
drivers/infiniband/core/core_priv.h +0 -1
@@ -294,7 +294,6 @@ static inline int ib_mad_enforce_security(struct ib_mad_agent_private *map, }#endif -struct ib_device *__ib_device_get_by_index (u32 ifindex);struct ib_device *ib_device_get_by_index (u32 ifindex);/* RDMA device netlink */ void nldev_init (void );
drivers/infiniband/core/device.c +4 -1
@@ -134,7 +134,10 @@ static int ib_device_check_mandatory(struct ib_device *device)return 0 ; } -struct ib_device *__ib_device_get_by_index (u32 index) +/* +* Caller to this function should hold lists_rwsem +*/ +static struct ib_device *__ib_device_get_by_index (u32 index) {struct ib_device *device;
drivers/infiniband/core/nldev.c +38 -24
129 lines not on this page · raw message
Signature · 6 lines
2.15.1 -- 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): 20180101110717.29686-7-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
Jason Gunthorpe
Reviewed-by Mark BlochRe: [PATCH rdma-next v1 6/7] RDMA/netlink: Protect device and port queries from device removal
drivers/infiniband/core/device.c static int ib_device_check_mandatory(struct ib_device *device) · line 139
Quoted 11 lines · Leon Romanovsky
drivers/infiniband/core/device.c +4
-1+++ b/drivers/infiniband/core/device.c@@ -134,7 +134,10 @@ static int ib_device_check_mandatory(struct ib_device *device)return 0 ; } -struct ib_device *__ib_device_get_by_index (u32 index) +/* +* Caller to this function should hold lists_rwsem +*/
This comment isn't 100% right, the device mutex is also OK to hold, I dropped it.
drivers/infiniband/core/device.c static int nldev_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh, · line 147
Quoted 10 lines
drivers/infiniband/core/device.c +2
-2@@ -141,36 +141,41 @@ static int nldev_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh,struct ib_device *device;struct sk_buff *msg;u32 index; -int err; +int ret = -ENOMEM ; - err =nlmsg_parse (nlh,0 , tb,RDMA_NLDEV_ATTR_MAX -1 , + ret =nlmsg_parse (nlh,0 , tb,RDMA_NLDEV_ATTR_MAX -1 , nldev_policy, extack);
Initializing ret, then overwriting it with nlmsg_parse is silly, and leads to this bug:
drivers/infiniband/core/device.c static int nldev_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh, · line 150
Quoted 4 lines
drivers/infiniband/core/device.c +1
-1msg =nlmsg_new (NLMSG_DEFAULT_SIZE ,GFP_KERNEL );if (!msg) -return -ENOMEM ; + goto err;
Wrong return code after the change.
I fixed it also dropped replacing err with ret since this is probably going to be backported.
drivers/infiniband/core/device.c static int nldev_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh, · line 153
Quoted 4 lines
drivers/infiniband/core/device.c +1
-1port =nla_get_u32 (tb[RDMA_NLDEV_ATTR_PORT_INDEX ]);if (!rdma_is_port_valid (device, port)) -return -EINVAL ; + goto err;
Same mistake again
I also squashed this with the prior patch to make backporting simpler
RDMA/core: Provide locked variant of device name to index function
And queued it to for-rc
See below, please check my work.
Author: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]> Date: Mon Jan 1 13:07:15 2018 +0200
RDMA/netlink: Fix locking around __ib_get_device_by_index
Holding locks is mandatory when calling __ib_device_get_by_index, otherwise there are races during the list iteration with device removal.
Since the locks are static to device.c, __ib_device_get_by_index can never be called correctly by any user out side the file.
Make the function static and provide a safe function that gets the correct locks and returns a kref'd pointer. Fix all callers.
drivers/infiniband/core/core_priv.h +1 -1
@@ -294,7 +294,7 @@ static inline int ib_mad_enforce_security(struct ib_mad_agent_private *map, }#endif -struct ib_device *__ib_device_get_by_index (u32 ifindex); +struct ib_device *ib_device_get_by_index (u32 ifindex);/* RDMA device netlink */ void nldev_init (void );void nldev_exit (void );
drivers/infiniband/core/device.c +17 -1
@@ -134,7 +134,7 @@ static int ib_device_check_mandatory(struct ib_device *device)return 0 ; } -struct ib_device *__ib_device_get_by_index (u32 index) +static struct ib_device *__ib_device_get_by_index (u32 index) {struct ib_device *device;
@@ -145,6 +145,22 @@ struct ib_device *__ib_device_get_by_index(u32 index)return NULL ; } +/* +* Caller is responsible to return refrerence count by calling put_device() +*/ +struct ib_device *ib_device_get_by_index (u32 index) +{ +struct ib_device *device; + +down_read (&lists_rwsem); + device =__ib_device_get_by_index (index); +if (device) +get_device (&device->dev); + +up_read (&lists_rwsem); +return device; +} +static struct ib_device *__ib_device_get_by_name (const char *name) {struct ib_device *device;
drivers/infiniband/core/nldev.c +36 -18
108 lines not on this page · raw message
--
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): 20180102211428.GA7831-uk2M96/[email protected]
Re: [PATCH rdma-next v1 6/7] RDMA/netlink: Protect device and port queries from device removal
Quoted 48 lines · Jason Gunthorpe
drivers/infiniband/core/device.c static int ib_device_check_mandatory(struct ib_device *device) · line 139
Quoted 11 lines · Leon Romanovsky
drivers/infiniband/core/device.c +4
-1+++ b/drivers/infiniband/core/device.c@@ -134,7 +134,10 @@ static int ib_device_check_mandatory(struct ib_device *device)return 0 ; } -struct ib_device *__ib_device_get_by_index (u32 index) +/* +* Caller to this function should hold lists_rwsem +*/ This comment isn't 100% right, the device mutex is also OK to hold, I dropped it.
drivers/infiniband/core/device.c static int nldev_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh, · line 147
Quoted 10 lines
drivers/infiniband/core/device.c +2
-2@@ -141,36 +141,41 @@ static int nldev_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh,struct ib_device *device;struct sk_buff *msg;u32 index; -int err; +int ret = -ENOMEM ; - err =nlmsg_parse (nlh,0 , tb,RDMA_NLDEV_ATTR_MAX -1 , + ret =nlmsg_parse (nlh,0 , tb,RDMA_NLDEV_ATTR_MAX -1 , nldev_policy, extack);Initializing ret, then overwriting it with
nlmsg_parseis silly, and leads to this bug:drivers/infiniband/core/device.c static int nldev_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh, · line 150
Quoted 4 lines
drivers/infiniband/core/device.c +1
-1msg =nlmsg_new (NLMSG_DEFAULT_SIZE ,GFP_KERNEL );if (!msg) -return -ENOMEM ; + goto err;Wrong return code after the change.
I fixed it also dropped replacing err with ret since this is probably going to be backported.
drivers/infiniband/core/device.c static int nldev_get_doit(struct sk_buff *skb, struct nlmsghdr *nlh, · line 153
Quoted 4 lines
drivers/infiniband/core/device.c +1
-1port =nla_get_u32 (tb[RDMA_NLDEV_ATTR_PORT_INDEX ]);if (!rdma_is_port_valid (device, port)) -return -EINVAL ; + goto err;Same mistake again
Quoted 7 lines
I also squashed this with the prior patch to make backporting simpler
RDMA/core: Provide locked variant of device name to index function
And queued it to for-rc
See below, please check my work.
Thanks for taking care.>
PATCH 7/7 RDMA/cma: Mark end of CMA ID messages
In reply to a message not in the archiveFrom: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
The commit 1a1c116f3dcf ("RDMA/netlink: Simplify the put_msg and put_attr") removes nlmsg_len calculation in ibnl_put_attr causing netlink messages and caused to miss source and destination addresses.
--- drivers/infiniband/core/cma.c | 1 + 1 file changed, 1 insertion(+)
drivers/infiniband/core/cma.c +1 -0
@@ -4440,6 +4440,7 @@ static int cma_get_id_stats(struct sk_buff *skb, struct netlink_callback *cb) id_stats->qp_type = id->qp_type; i_id++; +nlmsg_end (skb, nlh); } cb->args[1 ] =0 ;
--
2.15.1
--
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 6 lines · Leon Romanovsky
Leon Romanovsky (7):
RDMA/rxe: Remove useless export symbol RDMA/netlink: Simplify code of autoload modules RDMA/core: Replace open-coded variant ofput_deviceRDMA/nldev: Refactor nldev handle to be common function RDMA/cma: Mark end of CMA ID messages
Applied the above to for-next.
These two below got reworked and applied
Quoted 2 lines
RDMA/core: Provide locked variant of device name to index function RDMA/netlink: Protect device and port queries from device removal
Thanks,
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