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.
