Re: [Cluster-devel] remove kernel_setsockopt and kernel_getsockopt
From: David Laight Date: Thu, 14 May 2020 10:26:41 + > I doubt we are the one company with out-of-tree drivers > that use the kernel_socket interface. Not our problem.
Re: [Cluster-devel] remove kernel_setsockopt and kernel_getsockopt
From: David Laight Date: Thu, 14 May 2020 08:29:30 + > You need to export functions that do most of the socket options > for all protocols. If all in-tree users of this stuff are converted, there is no argument for keeping these routines. You seemed to be concerned about out of tree stuff. If so, that's not of our concern.
Re: [Cluster-devel] [Ocfs2-devel] remove kernel_setsockopt and kernel_getsockopt
On Thu, May 14, 2020 at 11:11:34AM +, David Laight wrote: > From: 'Christoph Hellwig' > > Sent: 14 May 2020 11:35 > > On Thu, May 14, 2020 at 10:26:41AM +, David Laight wrote: > > > From: Christoph Hellwig > > > > Only for those were we have users, and all those are covered. > > > > > > What do we tell all our users when our kernel SCTP code > > > no longer works? > > > > We only care about in-tree modules, just like for every other interface > > in the kernel. > > Even if our management agreed to release the code and the code > layout matched the kernel guidelines you still wouldn't want > two large drivers that implement telephony functionality > for hardware that very few people actually have. Oh, good point, we'll change the policy for all modules and make every interface in the kernel stable from now on to cater to your special case.
Re: [Cluster-devel] is it ok to always pull in sctp for dlm, was: Re: [PATCH 27/33] sctp: export sctp_setsockopt_bindx
On Thu, May 14, 2020 at 12:40:40PM +0200, Christoph Hellwig wrote: > On Wed, May 13, 2020 at 03:00:58PM -0300, Marcelo Ricardo Leitner wrote: > > On Wed, May 13, 2020 at 08:26:42AM +0200, Christoph Hellwig wrote: > > > And call it directly from dlm instead of going through kernel_setsockopt. > > > > The advantage on using kernel_setsockopt here is that sctp module will > > only be loaded if dlm actually creates a SCTP socket. With this > > change, sctp will be loaded on setups that may not be actually using > > it. It's a quite big module and might expose the system. > > > > I'm okay with the SCTP changes, but I'll defer to DLM folks to whether > > that's too bad or what for DLM. > > So for ipv6 I could just move the helpers inline as they were trivial > and avoid that issue. But some of the sctp stuff really is way too > big for that, so the only other option would be to use symbol_get. Let's try symbol_get, having the sctp module always loaded caused problems last time it happened (almost nobody uses dlm with it.) Dave
Re: [Cluster-devel] [PATCH 32/33] sctp: add sctp_sock_get_primary_addr
From: David Laight > Sent: 14 May 2020 13:30 > Subject: RE: [PATCH 32/33] sctp: add sctp_sock_get_primary_addr > > From: David Laight > > Sent: 14 May 2020 10:51 > > From: Marcelo Ricardo Leitner > > > Sent: 13 May 2020 19:03 > > > > > > On Wed, May 13, 2020 at 08:26:47AM +0200, Christoph Hellwig wrote: > > > > Add a helper to directly get the SCTP_PRIMARY_ADDR sockopt from kernel > > > > space without going through a fake uaccess. > > > > > > Same comment as on the other dlm/sctp patch. > > > > Wouldn't it be best to write sctp_[gs]etsockotp() that > > use a kernel buffer and then implement the user-space > > calls using a wrapper that does the copies to an on-stack > > (or malloced if big) buffer. > > Actually looking at __sys_setsockopt() it calls > BPF_CGROUP_RUN_PROG_SETSOCKOPT() which (by the look of it) > can copy the user buffer into malloc()ed memory and > cause set_fs(KERNEL_DS) be called. > > The only way to get rid of that set_fs() is to always > have the buffer in kernel memory when the underlying > setsockopt() code is called. And having started to try coding __sys_setsockopt() and then found the compat code I suspect that would be a whole lot more sane if the buffer was in kernel and it knew that at least (say) 64 bytes were allocated. The whole compat_alloc_user_space() 'crap' could probably go. Actually it looks like an application can avoid whatever checks BPF_CGROUP_RUN_PROG_SETSOCKOPT() is trying to do by using the 32bit compat ioctls. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [Cluster-devel] [PATCH 32/33] sctp: add sctp_sock_get_primary_addr
From: David Laight > Sent: 14 May 2020 10:51 > From: Marcelo Ricardo Leitner > > Sent: 13 May 2020 19:03 > > > > On Wed, May 13, 2020 at 08:26:47AM +0200, Christoph Hellwig wrote: > > > Add a helper to directly get the SCTP_PRIMARY_ADDR sockopt from kernel > > > space without going through a fake uaccess. > > > > Same comment as on the other dlm/sctp patch. > > Wouldn't it be best to write sctp_[gs]etsockotp() that > use a kernel buffer and then implement the user-space > calls using a wrapper that does the copies to an on-stack > (or malloced if big) buffer. Actually looking at __sys_setsockopt() it calls BPF_CGROUP_RUN_PROG_SETSOCKOPT() which (by the look of it) can copy the user buffer into malloc()ed memory and cause set_fs(KERNEL_DS) be called. The only way to get rid of that set_fs() is to always have the buffer in kernel memory when the underlying setsockopt() code is called. The comment above __sys_[sg]etsockopt() about not knowing the length is just wrong. It probably applied to getsockopt() in the dim and distant past before it was made read-update. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [Cluster-devel] [PATCH 20/33] ipv4: add ip_sock_set_recverr
On Thu, 2020-05-14 at 12:30 +0200, Christoph Hellwig wrote: > On Wed, May 13, 2020 at 02:00:43PM -0700, Joe Perches wrote: > > On Wed, 2020-05-13 at 08:26 +0200, Christoph Hellwig wrote: > > > Add a helper to directly set the IP_RECVERR sockopt from kernel space > > > without going through a fake uaccess. > > > > This seems used only with true as the second arg. > > Is there reason to have that argument at all? > > Mostly to keep it symmetric with the sockopt. I could probably remove > a few arguments in the series if we want to be strict. My preference would use strict and add arguments only when necessary.
Re: [Cluster-devel] remove kernel_setsockopt and kernel_getsockopt
From: 'Christoph Hellwig' > Sent: 14 May 2020 11:35 > On Thu, May 14, 2020 at 10:26:41AM +, David Laight wrote: > > From: Christoph Hellwig > > > Only for those were we have users, and all those are covered. > > > > What do we tell all our users when our kernel SCTP code > > no longer works? > > We only care about in-tree modules, just like for every other interface > in the kernel. Even if our management agreed to release the code and the code layout matched the kernel guidelines you still wouldn't want two large drivers that implement telephony functionality for hardware that very few people actually have. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
[Cluster-devel] is it ok to always pull in sctp for dlm, was: Re: [PATCH 27/33] sctp: export sctp_setsockopt_bindx
On Wed, May 13, 2020 at 03:00:58PM -0300, Marcelo Ricardo Leitner wrote: > On Wed, May 13, 2020 at 08:26:42AM +0200, Christoph Hellwig wrote: > > And call it directly from dlm instead of going through kernel_setsockopt. > > The advantage on using kernel_setsockopt here is that sctp module will > only be loaded if dlm actually creates a SCTP socket. With this > change, sctp will be loaded on setups that may not be actually using > it. It's a quite big module and might expose the system. > > I'm okay with the SCTP changes, but I'll defer to DLM folks to whether > that's too bad or what for DLM. So for ipv6 I could just move the helpers inline as they were trivial and avoid that issue. But some of the sctp stuff really is way too big for that, so the only other option would be to use symbol_get.
Re: [Cluster-devel] remove kernel_setsockopt and kernel_getsockopt
On Thu, May 14, 2020 at 10:26:41AM +, David Laight wrote: > From: Christoph Hellwig > > Only for those were we have users, and all those are covered. > > What do we tell all our users when our kernel SCTP code > no longer works? We only care about in-tree modules, just like for every other interface in the kernel.
Re: [Cluster-devel] [PATCH 20/33] ipv4: add ip_sock_set_recverr
On Wed, May 13, 2020 at 02:00:43PM -0700, Joe Perches wrote: > On Wed, 2020-05-13 at 08:26 +0200, Christoph Hellwig wrote: > > Add a helper to directly set the IP_RECVERR sockopt from kernel space > > without going through a fake uaccess. > > This seems used only with true as the second arg. > Is there reason to have that argument at all? Mostly to keep it symmetric with the sockopt. I could probably remove a few arguments in the series if we want to be strict.
Re: [Cluster-devel] [PATCH 29/33] rxrpc_sock_set_min_security_level
On Wed, May 13, 2020 at 02:13:07PM +0100, David Howells wrote: > Christoph Hellwig wrote: > > > +int rxrpc_sock_set_min_security_level(struct sock *sk, unsigned int val); > > + > > Looks good - but you do need to add this to Documentation/networking/rxrpc.txt > also, thanks. That file doesn't exist, instead we now have a cumentation/networking/rxrpc.rst in weird markup. Where do you want this to be added, and with what text? Remember I don't really know what this thing does, I just provide a shortcut.
Re: [Cluster-devel] remove kernel_setsockopt and kernel_getsockopt
From: Christoph Hellwig > Only for those were we have users, and all those are covered. What do we tell all our users when our kernel SCTP code no longer works? It uses SO_REUSADDR, SCTP_EVENTS, SCTP_NODELAY, SCTP_STATUS, SCTP_INITMSG, IPV6_ONLY, SCTP_SOCKOPT_BINDX_ADD and SO_LINGER. We should probably use the CONNECTX function as well. I doubt we are the one company with out-of-tree drivers that use the kernel_socket interface. 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
On Thu, May 14, 2020 at 08:29:30AM +, David Laight wrote: > You need to export functions that do most of the socket options > for all protocols. Only for those were we have users, and all those are covered.
Re: [Cluster-devel] [PATCH 32/33] sctp: add sctp_sock_get_primary_addr
From: Marcelo Ricardo Leitner > Sent: 13 May 2020 19:03 > > On Wed, May 13, 2020 at 08:26:47AM +0200, Christoph Hellwig wrote: > > Add a helper to directly get the SCTP_PRIMARY_ADDR sockopt from kernel > > space without going through a fake uaccess. > > Same comment as on the other dlm/sctp patch. Wouldn't it be best to write sctp_[gs]etsockotp() that use a kernel buffer and then implement the user-space calls using a wrapper that does the copies to an on-stack (or malloced if big) buffer. That will also simplify the code be removing all the copies and -EFAULT returns. Only the size checks will be needed and the code can assume the buffer is at least the size of the on-stack buffer. Our SCTP code uses SO_REUSADDR, SCTP_EVENTS, SCTP_NODELAY, SCTP_STATUS, SCTP_INITMSG, IPV6_ONLY, SCTP_SOCKOPT_BINDX_ADD and SO_LINGER. 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
From: Joe Perches > Sent: 13 May 2020 18:39 > On Wed, 2020-05-13 at 08:26 +0200, Christoph Hellwig wrote: > > 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 - counter to that a few kernel > > drivers just set the fields directly already. > > > > Nevertheless the diffstat looks quite promising: > > > > 42 files changed, 721 insertions(+), 799 deletions(-) I missed this patch going through. Massive NACK. You need to export functions that do most of the socket options for all protocols. As well as REUSADDR and NODELAY SCTP has loads because a lot of stuff that should have been extra system calls got piled into setsockopt. An alternate solution would be to move the copy_to/from_user() into a wrapper function so that the kernel_[sg]etsockopt() functions would bypass them completely. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [Cluster-devel] [PATCH 27/33] sctp: export sctp_setsockopt_bindx
From: Marcelo Ricardo Leitner > Sent: 13 May 2020 19:01 > On Wed, May 13, 2020 at 08:26:42AM +0200, Christoph Hellwig wrote: > > And call it directly from dlm instead of going through kernel_setsockopt. > > The advantage on using kernel_setsockopt here is that sctp module will > only be loaded if dlm actually creates a SCTP socket. With this > change, sctp will be loaded on setups that may not be actually using > it. It's a quite big module and might expose the system. > > I'm okay with the SCTP changes, but I'll defer to DLM folks to whether > that's too bad or what for DLM. I didn't see these sneak through. There is a big long list of SCTP socket options that are needed to make anything work. They all need exporting. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [Cluster-devel] [PATCH 27/33] sctp: export sctp_setsockopt_bindx
On Wed, May 13, 2020 at 03:00:58PM -0300, Marcelo Ricardo Leitner wrote: > On Wed, May 13, 2020 at 08:26:42AM +0200, Christoph Hellwig wrote: > > And call it directly from dlm instead of going through kernel_setsockopt. > > The advantage on using kernel_setsockopt here is that sctp module will > only be loaded if dlm actually creates a SCTP socket. With this > change, sctp will be loaded on setups that may not be actually using > it. It's a quite big module and might expose the system. True. Not that the intent is to kill kernel space callers of setsockopt, as I plan to remove the set_fs address space override used for it. So if always pulling in sctp is not an option for the DLM maintainers we'd have to do tricks using symbol_get() or similar. The same would also apply for ipv6, although I'm not sure how common modular ipv6 is in practice.
Re: [Cluster-devel] remove kernel_setsockopt and kernel_getsockopt
On Wed, May 13, 2020 at 10:38:59AM -0700, Joe Perches wrote: > It might be useful to show overall object size change. > > More EXPORT_SYMBOL uses increase object size a little. > > And not sure it matters much except it reduces overall object > size, but these patches remove (unnecessary) logging on error > and that could be mentioned in the cover letter too. The intent here is not to reduce code size. The intent is to kill of set_fs users so that we can eventually remove set_fs entirely.
Re: [Cluster-devel] [PATCH 21/33] ipv4: add ip_sock_set_mtu_discover
On Wed, May 13, 2020 at 02:17:41PM +0100, David Howells wrote: > Christoph Hellwig wrote: > > > + ip_sock_set_mtu_discover(conn->params.local->socket->sk, > > + IP_PMTUDISC_DONT); > > Um... The socket in question could be an AF_INET6 socket, not an AF_INET4 > socket - I presume it will work in that case. If so: Yes, the implementation of that sockopt, including the inet_sock structure where these options are set is shared between ipv4 and ipv6.
