netdev - RE: [PATCH rdma-next 11/19] RDMA/netlink: Convert LS to doit callback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 21 Jun 2017 09:18:25 -0500
From:   "Steve Wise" <swise@...ngridcomputing.com>
To:     "'Leon Romanovsky'" <leon@...nel.org>,
        "'Doug Ledford'" <dledford@...hat.com>
Cc:     <linux-rdma@...r.kernel.org>,
        "'Chien Tin Tung'" <chien.tin.tung@...el.com>,
        "'Stephen Hemminger'" <stephen@...workplumber.org>,
        "'Jiri Pirko'" <jiri@...lanox.com>,
        "'Ariel Almog'" <ariela@...lanox.com>,
        "'Linux Netdev'" <netdev@...r.kernel.org>,
        "'Leon Romanovsky'" <leonro@...lanox.com>
Subject: RE: [PATCH rdma-next 11/19] RDMA/netlink: Convert LS to doit callback

> From: Leon Romanovsky <leonro@...lanox.com>
> 
> RDMA_NL_LS protocol is actually is not dump anything,

nit:  "is not" -> "does not"

> but sets data and it should be handled by doit callback.
> 
> This patch actually converts RDMA_NL_LS to doit callback, while
> preserving IWCM and RDMA_CM flows through netlink_dump_start().
> 
> Signed-off-by: Leon Romanovsky <leonro@...lanox.com>



> ---
>  drivers/infiniband/core/addr.c      |  5 ++---
>  drivers/infiniband/core/core_priv.h |  9 ++++++---
>  drivers/infiniband/core/device.c    |  6 +++---
>  drivers/infiniband/core/netlink.c   | 28 ++++++++++------------------
>  drivers/infiniband/core/sa_query.c  |  8 ++++----
>  5 files changed, 25 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c
> index 2cc23a26ce4f..0b283029bc61 100644
> --- a/drivers/infiniband/core/addr.c
> +++ b/drivers/infiniband/core/addr.c
> @@ -129,10 +129,9 @@ static void ib_nl_process_good_ip_rsep(const struct
> nlmsghdr *nlh)
>  }
> 
>  int ib_nl_handle_ip_res_resp(struct sk_buff *skb,
> -			     struct netlink_callback *cb)
> +			     struct nlmsghdr *nlh,
> +			     struct netlink_ext_ack *extack)
>  {
> -	const struct nlmsghdr *nlh = (struct nlmsghdr *)cb->nlh;
> -
>  	if ((nlh->nlmsg_flags & NLM_F_REQUEST) ||
>  	    !(NETLINK_CB(skb).sk))
>  		return -EPERM;
> diff --git a/drivers/infiniband/core/core_priv.h
> b/drivers/infiniband/core/core_priv.h
> index 049ccbfca988..0137aa1ec471 100644
> --- a/drivers/infiniband/core/core_priv.h
> +++ b/drivers/infiniband/core/core_priv.h
> @@ -178,11 +178,14 @@ int ib_sa_init(void);
>  void ib_sa_cleanup(void);
> 
>  int ib_nl_handle_resolve_resp(struct sk_buff *skb,
> -			      struct netlink_callback *cb);
> +			      struct nlmsghdr *nlh,
> +			      struct netlink_ext_ack *extack);
>  int ib_nl_handle_set_timeout(struct sk_buff *skb,
> -			     struct netlink_callback *cb);
> +			     struct nlmsghdr *nlh,
> +			     struct netlink_ext_ack *extack);
>  int ib_nl_handle_ip_res_resp(struct sk_buff *skb,
> -			     struct netlink_callback *cb);
> +			     struct nlmsghdr *nlh,
> +			     struct netlink_ext_ack *extack);
> 
>  struct ib_device *__ib_device_get_by_name(const char *name);
>  #endif /* _CORE_PRIV_H */
> diff --git a/drivers/infiniband/core/device.c
b/drivers/infiniband/core/device.c
> index 4ec1b24258de..7317f203a315 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -1034,15 +1034,15 @@ EXPORT_SYMBOL(ib_get_net_dev_by_params);
> 
>  static const struct rdma_nl_cbs ibnl_ls_cb_table[] = {
>  	[RDMA_NL_LS_OP_RESOLVE] = {
> -		.dump = ib_nl_handle_resolve_resp,
> +		.doit = ib_nl_handle_resolve_resp,
>  		.flags = RDMA_NL_ADMIN_PERM,
>  	},
>  	[RDMA_NL_LS_OP_SET_TIMEOUT] = {
> -		.dump = ib_nl_handle_set_timeout,
> +		.doit = ib_nl_handle_set_timeout,
>  		.flags = RDMA_NL_ADMIN_PERM,
>  	},
>  	[RDMA_NL_LS_OP_IP_RESOLVE] = {
> -		.dump = ib_nl_handle_ip_res_resp,
> +		.doit = ib_nl_handle_ip_res_resp,
>  		.flags = RDMA_NL_ADMIN_PERM,
>  	},
>  };
> diff --git a/drivers/infiniband/core/netlink.c
b/drivers/infiniband/core/netlink.c
> index 73c74d1cd2a3..52fce73be9c1 100644
> --- a/drivers/infiniband/core/netlink.c
> +++ b/drivers/infiniband/core/netlink.c
> @@ -154,38 +154,30 @@ static int rdma_nl_rcv_msg(struct sk_buff *skb, struct
> nlmsghdr *nlh,
>  	int type = nlh->nlmsg_type;
>  	unsigned int index = RDMA_NL_GET_CLIENT(type);
>  	unsigned int op = RDMA_NL_GET_OP(type);
> -	struct netlink_callback cb = {};
> -	struct netlink_dump_control c = {};
>  	const struct rdma_nl_cbs *cb_table;
> -	int ret;
> 
>  	if (!is_nl_valid(index, op))
>  		return -EINVAL;
> 
> -	cb_table = rdma_nl_types[type].cb_table;
> +	cb_table = rdma_nl_types[index].cb_table;
> 
>  	if ((cb_table[op].flags & RDMA_NL_ADMIN_PERM) &&
>  	    !netlink_capable(skb, CAP_NET_ADMIN))
>  		return -EPERM;
> 
> -	/*
> -	 * For response or local service set_timeout request,
> -	 * there is no need to use netlink_dump_start.
> -	 */
> -	if (!(nlh->nlmsg_flags & NLM_F_REQUEST) ||
> -	    (index == RDMA_NL_LS && op == RDMA_NL_LS_OP_SET_TIMEOUT)) {
> -		cb.skb = skb;
> -		cb.nlh = nlh;
> -		cb.dump = cb_table[op].dump;
> -		return cb.dump(skb, &cb);
> -	} else {
> -		c.dump = cb_table[op].dump;
> +	/* TODO: Convert IWCM to properly handle doit callbacks */
> +	if ((nlh->nlmsg_flags & NLM_F_DUMP) || index == RDMA_NL_RDMA_CM
> ||
> +	    index == RDMA_NL_IWCM) {
> +		struct netlink_dump_control c = {
> +			.dump = cb_table[op].dump,
> +		};

Any reason you didn't fix IWCM as part of this series?  Or will you fix it in an
upcoming series?  Also, isn't FIXME: the norm for these sorts of "I don't want
to fix this now" comments?