Re: [Cluster-devel] [PATCH 32/33] net: add a new bind_add method
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
> ---
> 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_setsockop
Re: [Cluster-devel] [PATCH 32/33] net: add a new bind_add method
On Wed, May 20, 2020 at 08:00:25PM -0300, Marcelo Ricardo Leitner wrote: > > + 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. sctp_do_bind and thus this function only support a single address, as that is the only thing that the DLM code requires. I could move the user copy out of sctp_setsockopt_bindx and reuse that, but it is a rather rcane API. > > This patch will conflict with David's one, > [PATCH net-next] sctp: Pull the user copies out of the individual sockopt > functions. Do you have a link? A quick google search just finds your mail that I'm replying to. > (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. We generally only add operations for things that we actually use. bind_del is another logical op, but we can trivially add that when we need it.
Re: [Cluster-devel] [PATCH 32/33] net: add a new bind_add method
On Thu, May 21, 2020 at 10:42:24AM +0200, Christoph Hellwig wrote:
> On Wed, May 20, 2020 at 08:00:25PM -0300, Marcelo Ricardo Leitner wrote:
> > > + 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.
>
> sctp_do_bind and thus this function only support a single address, as
> that is the only thing that the DLM code requires. I could move the
I see.
> user copy out of sctp_setsockopt_bindx and reuse that, but it is a
> rather rcane API.
Yes. With David's patch, which is doing that, it can be as simple as:
static int sctp_bind_add(struct sock *sk, struct sockaddr *addr,
int addrlen)
{
int ret;
lock_sock(sk);
ret = sctp_setsockopt_bindx(sk, addr, addrlen, SCTP_BINDX_ADD_ADDR);
release_sock(sk);
return ret;
}
and then dlm would be using code that we can test through sctp-only tests as
well.
>
> >
> > This patch will conflict with David's one,
> > [PATCH net-next] sctp: Pull the user copies out of the individual sockopt
> > functions.
>
> Do you have a link? A quick google search just finds your mail that
> I'm replying to.
https://lore.kernel.org/netdev/fd94b5e41a7c4edc8f743c56a04ed2c9%40AcuMS.aculab.com/T/
>
> > (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.
>
> We generally only add operations for things that we actually use.
> bind_del is another logical op, but we can trivially add that when we
> need it.
Right, okay.
