Re: [PATCH net v4 1/2] tun: keep a kernel copy of the socket filter program

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Rongguang Wei wrote:
> From: Rongguang Wei <weirongguang@kylinos.cn>
> 
> TUNATTACHFILTER copies only the sock_fprog header, so tun->fprog.filter
> stays a pointer into the address space of the process that issued the
> ioctl. tun_attach() reads it again whenever a queue is attached to the
> persistent device later on.
> 
> Rebuilding the filter from that pointer is not reliable. With the inverted
> check below, a failed read (-EFAULT for an unmapped address, -EINVAL for
> bytes that are not a valid classic BPF program) was not fatal: for a new
> tfile err is overwritten by "err = 0", so the queue was attached without a
> filter. And when the read succeeded, the early return meant the queue was
> never published at all.
> 
> Keep the program in the kernel instead: tun->fprog_kern holds the
> instructions, and each queue gets its own program built from it with the
> new sk_attach_filter_kern(), the kernel memory counterpart of
> sk_attach_filter(). The copy is freed when the filter is detached or
> replaced and with the device, and tun->fprog is left untouched so
> TUNGETFILTER keeps its uapi behaviour.
> 
> Also fix the inverted check at the same time when tun_attach() returns
> early when sk_attach_filter_kern() succeeds instead of when it fails.
> Neither change works on its own: with only the kernel copy, every re-attach
> returns 0 without publishing the queue; with only the check fixed, a
> re-attach that used to succeed without installing a filter would start to
> fail.
> 
> sk_attach_filter_kern() builds the program with bpf_prog_create(), which
> does not keep an original program, so SO_GET_FILTER returns -EACCES and
> sock_diag omits the filter for sockets that use it. tun sockets are not
> exposed as file descriptors, so this is not user visible.
> 
> The inverted check was discovered by manual code inspection first [1], and
> the review of v1 reported the other things.
> 
> [1] https://lore.kernel.org/netdev/20260923025653.59348-1-clementwei90@163.com/
> 
> Fixes: 54f968d6efdb ("tuntap: move socket to tun_file")
> Link: https://lore.kernel.org/netdev/179027275318.2160803.4185895144088175048@kernel.org/
> Signed-off-by: Rongguang Wei <weirongguang@kylinos.cn>

Reviewed-by: Willem de Bruijn <willemb@google.com>