RE: [PATCH 0/5] add initial io_uring_cmd support for sockets

[Adrien Delorme <delorme.ade@outlook.com>]

From Adrien Delorme
> From : Pavel Begunkov 
> Sent : 2 May 2023 15:04
> On 5/2/23 10:21, Adrien Delorme wrote:
> >  From Adrien Delorme
> >
> >> From: David Ahern
> >> Sent: 12 April 2023 7:39
> >>> Sent: 11 April 2023 16:28
> >> ....
> >> One problem is that not all sockopt calls pass the correct length.
> >> And some of them can have very long buffers.
> >> Not to mention the ones that are read-modify-write.
> >>
> >> A plausible solution is to pass a 'fat pointer' that contains some,
> >> or all, of:
> >>        - A userspace buffer pointer.
> >>        - A kernel buffer pointer.
> >>        - The length supplied by the user.
> >>        - The length of the kernel buffer.
> >>        = The number of bytes to copy on completion.
> >> For simple user requests the syscall entry/exit code would copy the
> >> data to a short on-stack buffer.
> >> Kernel users just pass the kernel address.
> >> Odd requests can just use the user pointer.
> >>
> >> Probably needs accessors that add in an offset.
> >>
> >> It might also be that some of the problematic sockopt were in decnet
> >> - now removed.
> >
> > Hello everyone,
> >
> > I'm currently working on an implementation of {get,set} sockopt.
> > Since this thread is already talking about it, I hope that I replying at the
> correct place.
> 
> Hi Adrien, I believe Breno is working on set/getsockopt as well and had
> similar patches for awhile, but that would need for some problems to be
> solved first, e.g. try and decide whether it copies to a ptr as the syscall
> versions or would get/return optval directly in sqe/cqe. And also where to
> store bits that you pass in struct args_setsockopt_uring, and whether to rely
> on SQE128 or not.
> 

Hello Pavel,
That is good to hear. If possible I would like to provide some help. 
I looked at the getsockopt implementation. From what I'm seeing, I believe that it would be easier to copies to a ptr as the syscall.
The length of the output is usually 4 bytes (sometimes less) but in a lot of cases, this length is variable. Sometime it can even be bigger that the SQE128 ring.

Here is a non-exhaustive list of those cases : 
/net/ipv4/tcp.c : int do_tcp_getsockopt(...)
  - TCP_INFO : up to 240 bytes
  - TCP_CC_INFO and TCP_REPAIR_WINDOW : up to 20 bytes
  - TCP_CONGESTION and TCP_ULP : up to 16 bytes
  - TCP_ZEROCPOY_RECEIVE : up to 64 bytes  
/net/atm/commun.c : int vcc_getsockopt(...)
  - SO_ATMQOS : up to 88 bytes
  - SO_ATMPVC : up to 16 bytes
/net/ipv4/io_sockglue.c : int do_ip_getsockopt(...)
  - MCAST_MSFILTER : up to 144 bytes
  - IP_MSFILTER : 16 bytes minimum

I will look into setsockopt but I believe it might be the same. 
If needed I can also complete this list. 
However there are some cases where it is hard to determinate a maximum amount of bytes in advance. 

As to where the bytes should be stored I was thinking of either :
  - As a pointer in sqe->addr so the SQE128 is not needed 
  - In sqe->cmd as a struct but from my understanding, the SQE128 is needed
> 
> > My implementation is rather simple using a struct that will be used to pass
> the necessary info throught sqe->cmd.
> >
> > Here is my implementation based of kernel version 6.3 :
> > ...
> > +/* Struct to pass args for IO_URING_CMD_OP_GETSOCKOPT and
> > +IO_URING_CMD_OP_SETSOCKOPT operations */ struct
> > +args_setsockopt_uring{
> 
> The name of the structure is quite inconsistent with the rest. It's better to be
> io_[uring_]_sockopt_arg or some variation.
> 
> > +       int                             level;
> > +       int                     optname;
> > +       char __user *   user_optval;
> > +       int                     optlen;
> 
> That's uapi, there should not be __user, and field sizes should be more
> portable, i.e. use __u32, __u64, etc, look through the file.
> 
> Would need to look into the get/setsockopt implementation before saying
> anything about uring_cmd_{set,get}sockopt.
> ...
> Pavel Begunkov

Thank you for the review.
Adrien Delorme
--