Re: [Cluster-devel] [PATCH 31/33] sctp: add sctp_sock_set_nodelay
On Thu, May 21, 2020 at 10:33:48AM -0300, Marcelo Ricardo Leitner wrote: > With the patch there are now two ways of enabling nodelay. There is exactly one way to do for user applications, and one way for kernel drivers. (actually they could just set the field directly, which no one does for sctp, but for ipv4 a few do just that).
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.
Re: [Cluster-devel] [PATCH 31/33] sctp: add sctp_sock_set_nodelay
On Thu, May 21, 2020 at 10:34:42AM +0200, Christoph Hellwig wrote: > On Wed, May 20, 2020 at 08:39:13PM -0300, Marcelo Ricardo Leitner wrote: > > On Wed, May 20, 2020 at 04:23:55PM -0700, David Miller wrote: > > > From: Marcelo Ricardo Leitner > > > Date: Wed, 20 May 2020 20:10:01 -0300 > > > > > > > The duplication with sctp_setsockopt_nodelay() is quite silly/bad. > > > > Also, why have the 'true' hardcoded? It's what dlm uses, yes, but the > > > > API could be a bit more complete than that. > > > > > > The APIs are being designed based upon what in-tree users actually > > > make use of. We can expand things later if necessary. > > > > Sometimes expanding things later can be though, thus why the worry. > > But ok, I get it. Thanks. > > > > The comment still applies, though. (re the duplication) > > Where do you see duplication? > > sctp_setsockopt_nodelay does the following things: > > - verifies optlen, returns -EINVAL if it doesn't match > - calls get_user, returns -EFAULT on error > - converts the value from get_user to a boolean and assigns it >to sctp_sk(sk)->nodelay > - returns 0. > > sctp_sock_set_nodelay does: > > - call lock_sock > - assign true to sctp_sk(sk)->nodelay > - call release_sock > - does not return an error code With the patch there are now two ways of enabling nodelay. It may be just a boolean set today, but if one wants to probe on it or if we want to extend it with anything, say a debug msg, we have to do it in two (very different) places.
Re: [Cluster-devel] remove kernel_setsockopt and kernel_getsockopt v2
From: 'Christoph Hellwig' > Sent: 21 May 2020 10:12 ... > > I worried about whether getsockopt() should read the entire > > user buffer first. SCTP needs the some of it often (including a > > sockaddr_storage in one case), TCP needs it once. > > However the cost of reading a few words is small, and a big > > buffer probably needs setting to avoid leaking kernel > > memory if the structure has holes or fields that don't get set. > > Reading from userspace solves both issues. > > As mention in the thread on the last series: That was my first idea, but > we have way to many sockopts, especially in obscure protocols that just > hard code the size. The chance of breaking userspace in a way that can't > be fixed without going back to passing user pointers to get/setsockopt > is way to high to commit to such a change unfortunately. Right the syscall stubs probably can't do it. But the per-protocol ones can for the main protocols. I posted a patch for SCTP yesterday that removes 800 lines of source and 8k of object code. Even that needs a horrid bodge for one request where the length returned has to be less than the data copied! David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [Cluster-devel] remove kernel_setsockopt and kernel_getsockopt v2
On Thu, May 21, 2020 at 08:01:33AM +, David Laight wrote: > How much does this increase the kernel code by? 44 files changed, 660 insertions(+), 843 deletions(-) > You are also replicating a lot of code making it more > difficult to maintain. No, I specifically don't. > I don't think the performance of an socket option code > really matters - it is usually done once when a socket > is initialised and the other costs of establishing a > connection will dominate. > > Pulling the user copies outside the [gs]etsocksopt switch > statement not only reduces the code size (source and object) > and trivially allows kernel_[sg]sockopt() to me added to > the list of socket calls. > > It probably isn't possible to pull the usercopies right > out into the syscall wrapper because of some broken > requests. Please read through the previous discussion of the rationale and the options. We've been there before. > I worried about whether getsockopt() should read the entire > user buffer first. SCTP needs the some of it often (including a > sockaddr_storage in one case), TCP needs it once. > However the cost of reading a few words is small, and a big > buffer probably needs setting to avoid leaking kernel > memory if the structure has holes or fields that don't get set. > Reading from userspace solves both issues. As mention in the thread on the last series: That was my first idea, but we have way to many sockopts, especially in obscure protocols that just hard code the size. The chance of breaking userspace in a way that can't be fixed without going back to passing user pointers to get/setsockopt is way to high to commit to such a change unfortunately.
Re: [Cluster-devel] [PATCH 31/33] sctp: add sctp_sock_set_nodelay
On Thu, May 21, 2020 at 09:06:19AM +, David Laight wrote: > > > The comment still applies, though. (re the duplication) > > > > Where do you see duplication? > > The whole thing just doesn't scale. > > As soon as you get to the slightly more complex requests > like SCTP_INITMSG (which should probably be called to > set the required number of data streams) you've either > got replicated code or nested wrappers. None of that is relevant to setting the nodelay option. If you actually read through the series you'd say that whenever there was non-trivial logic it is shared with getopt. However sharing just for purpose of sharing doesn't make sense, so where the kernel API ended up just setting a flag after taking the sock lock I did not opt for it.
Re: [Cluster-devel] [PATCH 31/33] sctp: add sctp_sock_set_nodelay
From: Christoph Hellwig > Sent: 21 May 2020 09:35 > On Wed, May 20, 2020 at 08:39:13PM -0300, Marcelo Ricardo Leitner wrote: > > On Wed, May 20, 2020 at 04:23:55PM -0700, David Miller wrote: > > > From: Marcelo Ricardo Leitner > > > Date: Wed, 20 May 2020 20:10:01 -0300 > > > > > > > The duplication with sctp_setsockopt_nodelay() is quite silly/bad. > > > > Also, why have the 'true' hardcoded? It's what dlm uses, yes, but the > > > > API could be a bit more complete than that. > > > > > > The APIs are being designed based upon what in-tree users actually > > > make use of. We can expand things later if necessary. > > > > Sometimes expanding things later can be though, thus why the worry. > > But ok, I get it. Thanks. > > > > The comment still applies, though. (re the duplication) > > Where do you see duplication? The whole thing just doesn't scale. As soon as you get to the slightly more complex requests like SCTP_INITMSG (which should probably be called to set the required number of data streams) you've either got replicated code or nested wrappers. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
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 31/33] sctp: add sctp_sock_set_nodelay
On Wed, May 20, 2020 at 08:39:13PM -0300, Marcelo Ricardo Leitner wrote: > On Wed, May 20, 2020 at 04:23:55PM -0700, David Miller wrote: > > From: Marcelo Ricardo Leitner > > Date: Wed, 20 May 2020 20:10:01 -0300 > > > > > The duplication with sctp_setsockopt_nodelay() is quite silly/bad. > > > Also, why have the 'true' hardcoded? It's what dlm uses, yes, but the > > > API could be a bit more complete than that. > > > > The APIs are being designed based upon what in-tree users actually > > make use of. We can expand things later if necessary. > > Sometimes expanding things later can be though, thus why the worry. > But ok, I get it. Thanks. > > The comment still applies, though. (re the duplication) Where do you see duplication? sctp_setsockopt_nodelay does the following things: - verifies optlen, returns -EINVAL if it doesn't match - calls get_user, returns -EFAULT on error - converts the value from get_user to a boolean and assigns it to sctp_sk(sk)->nodelay - returns 0. sctp_sock_set_nodelay does: - call lock_sock - assign true to sctp_sk(sk)->nodelay - call release_sock - does not return an error code
Re: [Cluster-devel] remove kernel_setsockopt and kernel_getsockopt v2
From: Christoph Hellwig > Sent: 20 May 2020 20:55 > > this series removes the kernel_setsockopt and kernel_getsockopt > functions, and instead switches their users to small functions that > implement setting (or in one case getting) a sockopt directly using > a normal kernel function call with type safety and all the other > benefits of not having a function call. > > In some cases these functions seem pretty heavy handed as they do > a lock_sock even for just setting a single variable, but this mirrors > the real setsockopt implementation unlike a few drivers that just set > set the fields directly. How much does this increase the kernel code by? You are also replicating a lot of code making it more difficult to maintain. I don't think the performance of an socket option code really matters - it is usually done once when a socket is initialised and the other costs of establishing a connection will dominate. Pulling the user copies outside the [gs]etsocksopt switch statement not only reduces the code size (source and object) and trivially allows kernel_[sg]sockopt() to me added to the list of socket calls. It probably isn't possible to pull the usercopies right out into the syscall wrapper because of some broken requests. I worried about whether getsockopt() should read the entire user buffer first. SCTP needs the some of it often (including a sockaddr_storage in one case), TCP needs it once. However the cost of reading a few words is small, and a big buffer probably needs setting to avoid leaking kernel memory if the structure has holes or fields that don't get set. Reading from userspace solves both issues. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [Cluster-devel] [PATCH 29/33] rxrpc: add rxrpc_sock_set_min_security_level
Christoph Hellwig wrote: > Add a helper to directly set the RXRPC_MIN_SECURITY_LEVEL sockopt from > kernel space without going through a fake uaccess. > > Thanks to David Howells for the documentation updates. > > Signed-off-by: Christoph Hellwig Acked-by: David Howells
