Re: [RFC PATCH 3/4] net: pass a kernel pointer via 'optlen_t' to proto[ops].getsockopt() hooks

[David Laight <david.laight.linux@gmail.com>]

On Mon, 31 Mar 2025 22:10:55 +0200
Stefan Metzmacher <metze@samba.org> wrote:

> The motivation for this is to remove the SOL_SOCKET limitation
> from io_uring_cmd_getsockopt().
> 
> The reason for this limitation is that io_uring_cmd_getsockopt()
> passes a kernel pointer.
> 
> The first idea would be to change the optval and optlen arguments
> to the protocol specific hooks also to sockptr_t, as that
> is already used for setsockopt() and also by do_sock_getsockopt()
> sk_getsockopt() and BPF_CGROUP_RUN_PROG_GETSOCKOPT().
> 
> But as Linus don't like 'sockptr_t' I used a different approach.
> 
> Instead of passing the optlen as user or kernel pointer,
> we only ever pass a kernel pointer and do the
> translation from/to userspace in do_sock_getsockopt().
> 
> The simple solution would be to just remove the
> '__user' from the int *optlen argument, but it
> seems the compiler doesn't complain about
> '__user' vs. without it, so instead I used
> a helper struct in order to make sure everything
> compiles with a typesafe change.
> 
> That together with get_optlen() and put_optlen() helper
> macros make it relatively easy to review and check the
> behaviour is most likely unchanged.

I've looked into this before (and fallen down the patch rabbit hole).

I think the best (final) solution is to pass a validated non-negative
'optlen' into all getsockopt() functions and to have them usually return
either -errno or the modified length.
This simplifies 99% of the functions.

The problem case is functions that want to update the length and return
an error.
By best solution is to support return values of -errno << 20 | length
(as well as -errno and length).

There end up being some slight behaviour changes.
- Some code tries to 'undo' actions if the length can't be updated.
  I'm sure this is unnecessary and the recovery path is untested and
  could be buggy. Provided the kernel data is consistent there is
  no point trying to get code to recover from EFAULT.
  The 'length' has been read - so would also need to be readonly
  or unmapped by a second thread!
- A lot of getsockopt functions actually treat a negative length as 4.
  I think this 'bug' needs to preserved to avoid breaking applications.

The changes are mechanical but very widespread.

They also give the option of not writing back the length if unchanged.

	David