Linux-NVME Archive on lore.kernel.org
 help / color / Atom feed
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Eric Dumazet <edumazet@google.com>,
	linux-nvme@lists.infradead.org, linux-sctp@vger.kernel.org,
	target-devel@vger.kernel.org, linux-afs@lists.infradead.org,
	drbd-dev@lists.linbit.com, linux-cifs@vger.kernel.org,
	rds-devel@oss.oracle.com, linux-rdma@vger.kernel.org,
	cluster-devel@redhat.com, Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>,
	Jakub Kicinski <kuba@kernel.org>,
	ceph-devel@vger.kernel.org, linux-nfs@vger.kernel.org,
	Neil Horman <nhorman@tuxdriver.com>,
	Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
	netdev@vger.kernel.org, Vlad Yasevich <vyasevich@gmail.com>,
	linux-kernel@vger.kernel.org, Jon Maloy <jmaloy@redhat.com>,
	Ying Xue <ying.xue@windriver.com>,
	"David S. Miller" <davem@davemloft.net>,
	ocfs2-devel@oss.oracle.com
Subject: Re: [PATCH 32/33] net: add a new bind_add method
Date: Wed, 20 May 2020 20:00:25 -0300
Message-ID: <20200520230025.GT2491@localhost.localdomain> (raw)
In-Reply-To: <20200520195509.2215098-33-hch@lst.de>

On Wed, May 20, 2020 at 09:55:08PM +0200, Christoph Hellwig wrote:
> The SCTP protocol allows to bind multiple address to a socket.  That
> feature is currently only exposed as a socket option.  Add a bind_add
> method struct proto that allows to bind additional addresses, and
> switch the dlm code to use the method instead of going through the
> socket option from kernel space.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/dlm/lowcomms.c  |  9 +++------
>  include/net/sock.h |  6 +++++-
>  net/core/sock.c    |  8 ++++++++
>  net/sctp/socket.c  | 23 +++++++++++++++++++++++
>  4 files changed, 39 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
> index 9f1c3cdc9d653..3543a8fec9075 100644
> --- a/fs/dlm/lowcomms.c
> +++ b/fs/dlm/lowcomms.c
> @@ -882,6 +882,7 @@ static void writequeue_entry_complete(struct writequeue_entry *e, int completed)
>  static int sctp_bind_addrs(struct connection *con, uint16_t port)
>  {
>  	struct sockaddr_storage localaddr;
> +	struct sockaddr *addr = (struct sockaddr *)&localaddr;
>  	int i, addr_len, result = 0;
>  
>  	for (i = 0; i < dlm_local_count; i++) {
> @@ -889,13 +890,9 @@ static int sctp_bind_addrs(struct connection *con, uint16_t port)
>  		make_sockaddr(&localaddr, port, &addr_len);
>  
>  		if (!i)
> -			result = kernel_bind(con->sock,
> -					     (struct sockaddr *)&localaddr,
> -					     addr_len);
> +			result = kernel_bind(con->sock, addr, addr_len);
>  		else
> -			result = kernel_setsockopt(con->sock, SOL_SCTP,
> -						   SCTP_SOCKOPT_BINDX_ADD,
> -						   (char *)&localaddr, addr_len);
> +			result = sock_bind_add(con->sock->sk, addr, addr_len);
>  
>  		if (result < 0) {
>  			log_print("Can't bind to %d addr number %d, %d.\n",
> diff --git a/include/net/sock.h b/include/net/sock.h
> index d994daa418ec2..6e9f713a78607 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1156,7 +1156,9 @@ struct proto {
>  	int			(*sendpage)(struct sock *sk, struct page *page,
>  					int offset, size_t size, int flags);
>  	int			(*bind)(struct sock *sk,
> -					struct sockaddr *uaddr, int addr_len);
> +					struct sockaddr *addr, int addr_len);
> +	int			(*bind_add)(struct sock *sk,
> +					struct sockaddr *addr, int addr_len);
>  
>  	int			(*backlog_rcv) (struct sock *sk,
>  						struct sk_buff *skb);
> @@ -2698,4 +2700,6 @@ void sock_set_reuseaddr(struct sock *sk);
>  void sock_set_reuseport(struct sock *sk);
>  void sock_set_sndtimeo(struct sock *sk, s64 secs);
>  
> +int sock_bind_add(struct sock *sk, struct sockaddr *addr, int addr_len);
> +
>  #endif	/* _SOCK_H */
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 2ca3425b519c0..61ec573221a60 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -3712,3 +3712,11 @@ bool sk_busy_loop_end(void *p, unsigned long start_time)
>  }
>  EXPORT_SYMBOL(sk_busy_loop_end);
>  #endif /* CONFIG_NET_RX_BUSY_POLL */
> +
> +int sock_bind_add(struct sock *sk, struct sockaddr *addr, int addr_len)
> +{
> +	if (!sk->sk_prot->bind_add)
> +		return -EOPNOTSUPP;
> +	return sk->sk_prot->bind_add(sk, addr, addr_len);
> +}
> +EXPORT_SYMBOL(sock_bind_add);
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 827a9903ee288..8a0b9258f65c0 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1057,6 +1057,27 @@ static int sctp_setsockopt_bindx(struct sock *sk,
>  	return err;
>  }
>  
> +static int sctp_bind_add(struct sock *sk, struct sockaddr *addr,
> +		int addrlen)
> +{
> +	struct sctp_af *af = sctp_get_af_specific(addr->sa_family);
> +	int err;
> +
> +	if (!af || af->sockaddr_len > addrlen)
> +		return -EINVAL;
> +	err = security_sctp_bind_connect(sk, SCTP_SOCKOPT_BINDX_ADD, addr,
> +			addrlen);

The security_ call above is done today within the sock lock.
I couldn't find any issue through a code review, though, so I'm fine
with leaving it as is. Just highlighting it..

> +	if (err)
> +		return err;
> +
> +	lock_sock(sk);
> +	err = sctp_do_bind(sk, (union sctp_addr *)addr, af->sockaddr_len);
> +	if (!err)
> +		err = sctp_send_asconf_add_ip(sk, addr, 1);

Some problems here.
- addr may contain a list of addresses
- the addresses, then, are not being validated
- sctp_do_bind may fail, on which it requires some undoing
  (like sctp_bindx_add does)
- code duplication with sctp_setsockopt_bindx.

This patch will conflict with David's one,
[PATCH net-next] sctp: Pull the user copies out of the individual sockopt functions.
(I'll finish reviewing it in the sequence)

AFAICT, this patch could reuse/build on his work in there. The goal is
pretty much the same and would avoid the issues above.

This patch could, then, point the new bind_add proto op to the updated
sctp_setsockopt_bindx almost directly.

Question then is: dlm never removes an addr from the bind list. Do we
want to add ops for both? Or one that handles both operations?
Anyhow, having the add operation but not the del seems very weird to
me.

> +	release_sock(sk);
> +	return err;
> +}
> +
>  static int sctp_connect_new_asoc(struct sctp_endpoint *ep,
>  				 const union sctp_addr *daddr,
>  				 const struct sctp_initmsg *init,
> @@ -9625,6 +9646,7 @@ struct proto sctp_prot = {
>  	.sendmsg     =	sctp_sendmsg,
>  	.recvmsg     =	sctp_recvmsg,
>  	.bind        =	sctp_bind,
> +	.bind_add    =  sctp_bind_add,
>  	.backlog_rcv =	sctp_backlog_rcv,
>  	.hash        =	sctp_hash,
>  	.unhash      =	sctp_unhash,
> @@ -9667,6 +9689,7 @@ struct proto sctpv6_prot = {
>  	.sendmsg	= sctp_sendmsg,
>  	.recvmsg	= sctp_recvmsg,
>  	.bind		= sctp_bind,
> +	.bind_add	= sctp_bind_add,
>  	.backlog_rcv	= sctp_backlog_rcv,
>  	.hash		= sctp_hash,
>  	.unhash		= sctp_unhash,
> -- 
> 2.26.2
> 

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

  reply index

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-20 19:54 remove kernel_setsockopt and kernel_getsockopt v2 Christoph Hellwig
2020-05-20 19:54 ` [PATCH 01/33] dlm: use the tcp version of accept_from_sock for sctp as well Christoph Hellwig
2020-05-20 19:54 ` [PATCH 02/33] net: remove kernel_getsockopt Christoph Hellwig
2020-05-20 19:54 ` [PATCH 03/33] net: add sock_set_reuseaddr Christoph Hellwig
2020-05-20 19:54 ` [PATCH 04/33] net: add sock_no_linger Christoph Hellwig
2020-05-20 19:54 ` [PATCH 05/33] net: add sock_set_priority Christoph Hellwig
2020-05-20 19:54 ` [PATCH 06/33] net: add sock_set_sndtimeo Christoph Hellwig
2020-05-20 19:54 ` [PATCH 07/33] net: add sock_bindtoindex Christoph Hellwig
2020-05-20 19:54 ` [PATCH 08/33] net: add sock_enable_timestamps Christoph Hellwig
2020-05-20 19:54 ` [PATCH 09/33] net: add sock_set_keepalive Christoph Hellwig
2020-05-20 19:54 ` [PATCH 10/33] net: add sock_set_rcvbuf Christoph Hellwig
2020-05-20 19:54 ` [PATCH 11/33] net: add sock_set_reuseport Christoph Hellwig
2020-05-20 19:54 ` [PATCH 12/33] tcp: add tcp_sock_set_cork Christoph Hellwig
2020-05-20 19:54 ` [PATCH 13/33] tcp: add tcp_sock_set_nodelay Christoph Hellwig
2020-05-20 19:54 ` [PATCH 14/33] tcp: add tcp_sock_set_quickack Christoph Hellwig
2020-05-20 19:54 ` [PATCH 15/33] tcp: add tcp_sock_set_syncnt Christoph Hellwig
2020-05-20 19:54 ` [PATCH 16/33] tcp: add tcp_sock_set_user_timeout Christoph Hellwig
2020-05-20 19:54 ` [PATCH 17/33] tcp: add tcp_sock_set_keepidle Christoph Hellwig
2020-05-20 19:54 ` [PATCH 18/33] tcp: add tcp_sock_set_keepintvl Christoph Hellwig
2020-05-20 19:54 ` [PATCH 19/33] tcp: add tcp_sock_set_keepcnt Christoph Hellwig
2020-05-20 19:54 ` [PATCH 20/33] ipv4: add ip_sock_set_tos Christoph Hellwig
2020-05-20 19:54 ` [PATCH 21/33] ipv4: add ip_sock_set_freebind Christoph Hellwig
2020-05-20 19:54 ` [PATCH 22/33] ipv4: add ip_sock_set_recverr Christoph Hellwig
2020-05-20 19:54 ` [PATCH 23/33] ipv4: add ip_sock_set_mtu_discover Christoph Hellwig
2020-05-20 19:55 ` [PATCH 24/33] ipv4: add ip_sock_set_pktinfo Christoph Hellwig
2020-05-20 19:55 ` [PATCH 25/33] ipv6: add ip6_sock_set_v6only Christoph Hellwig
2020-05-20 19:55 ` [PATCH 26/33] ipv6: add ip6_sock_set_recverr Christoph Hellwig
2020-05-20 19:55 ` [PATCH 27/33] ipv6: add ip6_sock_set_addr_preferences Christoph Hellwig
2020-05-20 19:55 ` [PATCH 28/33] ipv6: add ip6_sock_set_recvpktinfo Christoph Hellwig
2020-05-20 19:55 ` [PATCH 29/33] rxrpc: add rxrpc_sock_set_min_security_level Christoph Hellwig
2020-05-20 19:55 ` [PATCH 30/33] tipc: call tsk_set_importance from tipc_topsrv_create_listener Christoph Hellwig
2020-05-20 19:55 ` [PATCH 31/33] sctp: add sctp_sock_set_nodelay Christoph Hellwig
2020-05-20 23:10   ` Marcelo Ricardo Leitner
2020-05-20 23:23     ` David Miller
2020-05-20 23:39       ` Marcelo Ricardo Leitner
2020-05-21  8:34         ` Christoph Hellwig
2020-05-21  9:06           ` David Laight
2020-05-21  9:08             ` 'Christoph Hellwig'
2020-05-21 13:33           ` Marcelo Ricardo Leitner
2020-05-21 13:57             ` Christoph Hellwig
2020-05-20 19:55 ` [PATCH 32/33] net: add a new bind_add method Christoph Hellwig
2020-05-20 23:00   ` Marcelo Ricardo Leitner [this message]
2020-05-21  8:42     ` Christoph Hellwig
2020-05-21 13:54       ` Marcelo Ricardo Leitner
2020-05-20 19:55 ` [PATCH 33/33] net: remove kernel_setsockopt Christoph Hellwig
2020-05-21  7:44 ` [PATCH 29/33] rxrpc: add rxrpc_sock_set_min_security_level David Howells
2020-05-21  8:01 ` remove kernel_setsockopt and kernel_getsockopt v2 David Laight
2020-05-21  9:11   ` 'Christoph Hellwig'
2020-05-21 10:46     ` David Laight
2020-05-23  7:23 ` Christoph Hellwig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200520230025.GT2491@localhost.localdomain \
    --to=marcelo.leitner@gmail.com \
    --cc=ceph-devel@vger.kernel.org \
    --cc=cluster-devel@redhat.com \
    --cc=davem@davemloft.net \
    --cc=drbd-dev@lists.linbit.com \
    --cc=edumazet@google.com \
    --cc=hch@lst.de \
    --cc=jmaloy@redhat.com \
    --cc=kuba@kernel.org \
    --cc=kuznet@ms2.inr.ac.ru \
    --cc=linux-afs@lists.infradead.org \
    --cc=linux-cifs@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=linux-nvme@lists.infradead.org \
    --cc=linux-rdma@vger.kernel.org \
    --cc=linux-sctp@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=nhorman@tuxdriver.com \
    --cc=ocfs2-devel@oss.oracle.com \
    --cc=rds-devel@oss.oracle.com \
    --cc=target-devel@vger.kernel.org \
    --cc=vyasevich@gmail.com \
    --cc=ying.xue@windriver.com \
    --cc=yoshfuji@linux-ipv6.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-NVME Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nvme/0 linux-nvme/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nvme linux-nvme/ https://lore.kernel.org/linux-nvme \
		linux-nvme@lists.infradead.org
	public-inbox-index linux-nvme

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-nvme


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git