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

[Rongguang Wei <clementwei90@163.com>]

on 2026/10/1 22:10, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 (2) · Low: 3 (1)
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] After this patch alone, tun_attach() in drivers/net/tun.c still
>   has the inverted check `if (!err) goto out;` right after `err =
Make the check first is not an option either (it would turn an attach 
that used to succeed without a filter into a failure). It seems the only
way is that the fix be folded into this patch, so no step of the series
takes goto out with err == 0.
> - [Low] The commit message gets both the old failure mode and the new
>   helper's semantics wrong.
> - [Low] The new exported helper sk_attach_filter_kern()
>   (net/core/filter.c) builds the program with bpf_prog_create() and then…
> - [Low] The new comment above tun_copy_filter()
>   (drivers/net/tun.c:726-729) says the kernel copy lets the filter "be…
I will reword it.
> 
> Pre-existing issues:
> - [Medium] tun has no compat translation for the classic filter ioctls.
I can send the compat translation as a separate patch.
> - [Medium] If TUNATTACHFILTER replaces an attached filter and
>   sk_attach_filter_kern() fails on queue i (for example -ENOMEM from…
The old code also overwrote tun->fprog before attaching, and the prefix-only
rollback is unchanged. A proper fix is to install the new copy on every queue 
and only then replace the saved one. Separate patch.
> - [Low] When TUNATTACHFILTER is issued while tun->numqueues == 0, the
>   classic BPF program is stored without ever being validated.The old code stored an unvalidated header in the same situation. 
Validating the program at copy time can be a separate change.
> 
> --
> 
> tun: keep a kernel copy of the socket filter program
> 
> This patch changes TUNATTACHFILTER to copy the classic BPF instructions
> into tun->fprog_kern. Before, only the sock_fprog header was stored, and
> it pointed into the ioctl caller's address space. It also adds a new
> exported helper, sk_attach_filter_kern(), which builds and attaches a
> program from a kernel sock_fprog_kern, and tun_attach() and
> tun_attach_filter() now use it.
> 
> Link: https://lore.kernel.org/netdev/179027275318.2160803.4185895144088175048@kernel.org/
> 
>> tun_attach() reads it again whenever a queue is attached to the
>> persistent device later on: unmapped there, the attach fails with -EFAULT;
>> mapped, whatever bytes it holds become the filter of the new queue.
> 
> [Severity: Low]
> Is this an accurate description of the old behaviour? tun_attach() has an
> inverted "if (!err) goto out;", so an -EFAULT, or an -EINVAL caused by
> garbage bytes, falls through instead of failing the attach.
> 
> For a new tfile, xdp_rxq_info_reg() and "err = 0" then overwrite err. So
> TUNSETIFF succeeds, the queue is attached, and no filter is installed.
> 
> A successful read of mapped, valid bytes returns 0 without attaching the
> queue at all.
> 
> The follow-up patch's message talks about "a re-attach that used to
> succeed without installing any filter". That seems to contradict this
> one.
> 
>> sk_attach_filter_kern() reads no user buffer and attaches the program like
>> sk_attach_filter() does.
> 
> Do the two really behave the same way? sk_attach_filter_kern() builds the
> program with bpf_prog_create(), which sets fp->orig_prog = NULL.
> sk_attach_filter() goes through __get_filter()->bpf_prog_store_orig_filter()
> instead. As a result, SO_GET_FILTER and sock_diag readback give different
> results depending on which helper attached the filter.
> 
>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>> index 5a302709a68aa..22fb34e745bd9 100644
>> --- a/drivers/net/tun.c
>> +++ b/drivers/net/tun.c
> 
> [ ... ]
> 
>> @@ -722,6 +723,34 @@ static void tun_force_wake_queue(struct tun_struct *tun,
>>  	spin_unlock_bh(&tfile->tx_ring.consumer_lock);
>>  }
>>  
>> +/* Copy the filter that @argp points at into the kernel, so that it can be
>> + * installed again later, from any context. tun->fprog and tun->fprog_kern
>> + * are updated only once the copy succeeded.
>> + */
> 
> [Severity: Low]
> Is "from any context" accurate here? The only consumer,
> sk_attach_filter_kern(), needs sleepable process context and the socket
> lock:
> 
> sk_attach_filter_kern()
>   bpf_prog_create()
>     bpf_prog_alloc()          <- GFP_KERNEL based allocation
>   __sk_attach_prog()
>     kmalloc_obj()             <- GFP_KERNEL
>     rcu_dereference_protected(..., lockdep_sock_is_held(sk))
> 
> Was the intended meaning "from any process", or "independent of the ioctl
> caller's address space"?
> 
>> +static int tun_copy_filter(struct tun_struct *tun, struct sock_fprog __user *argp)
>> +{
>> +	struct sock_fprog fprog;
>> +	struct sock_filter *insns;
>> +
>> +	if (copy_from_user(&fprog, argp, sizeof(fprog)))
>> +		return -EFAULT;
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but tun has no compat handling
> for the classic filter ioctls. TUNATTACHFILTER, TUNDETACHFILTER and
> TUNGETFILTER encode sizeof(struct sock_fprog) in the command number. That
> size is 8 bytes for 32-bit userspace and 16 bytes natively, so the command
> numbers differ.
> 
> tun_chr_compat_ioctl() only converts these:
> 
> 	case TUNSETIFF:
> 	case TUNGETIFF:
> 	case TUNSETTXFILTER:
> 	case TUNGETSNDBUF:
> 	case TUNSETSNDBUF:
> 	case SIOCGIFHWADDR:
> 	case SIOCSIFHWADDR:
> 		arg = (unsigned long)compat_ptr(arg);
> 		break;
> 
> The compat filter commands therefore miss the native cases in
> __tun_chr_ioctl() and end up in tun_vnet_ioctl(), which returns -EINVAL.
> tun_copy_filter() is never reached with a compat layout. The result is
> that 32-bit userspace on a 64-bit kernel cannot use these ioctls.
> 
> This copy is being rewritten anyway. Would it be worth adding compat
> command translation here, perhaps together with the compat-aware
> copy_bpf_fprog_from_user() that SO_ATTACH_FILTER uses?
> 
>> +
>> +	if (!fprog.len || fprog.len > BPF_MAXINSNS)
>> +		return -EINVAL;
> 
> [Severity: Low]
> This is a pre-existing issue, but only the length is checked before the
> program is committed to tun->fprog_kern. If TUNATTACHFILTER is issued
> while tun->numqueues == 0, does the program ever get validated?
> 
> A multiqueue TAP reaches that state after TUNSETQUEUE(IFF_DETACH_QUEUE) on
> every queue. __tun_detach(tfile, false) lowers numqueues but keeps
> tfile->tun, so tun_get() still succeeds in __tun_chr_ioctl().
> 
> In that case the loop in tun_attach_filter() runs zero times, and
> bpf_check_classic() (via bpf_prog_create()->bpf_prepare_filter()) never
> runs. filter_attached is still set to true and 0 is returned.
> 
> After that, every tun_attach() fails in sk_attach_filter_kern() with
> -EINVAL until TUNDETACHFILTER. The old code had the same gap: it stored an
> unvalidated header when numqueues == 0.
> 
>> +
>> +	insns = memdup_array_user(fprog.filter, fprog.len,
>> +				  sizeof(struct sock_filter));
> 
> [ ... ]
> 
>> @@ -752,7 +781,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
>>  	/* Re-attach the filter to persist device */
>>  	if (!skip_filter && (tun->filter_attached == true)) {
>>  		lock_sock(tfile->socket.sk);
>> -		err = sk_attach_filter(&tun->fprog, tfile->socket.sk);
>> +		err = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
>>  		release_sock(tfile->socket.sk);
>>  		if (!err)
>>  			goto out;
> 
> [Severity: High]
> With only this patch applied, is the check after sk_attach_filter_kern()
> still inverted?
> 
> The program is now built from a kernel copy, so the build succeeds for
> any valid program. On a persistent TAP with filter_attached set, TUNSETIFF
> without IFF_NOFILTER and TUNSETQUEUE(IFF_ATTACH_QUEUE) would then jump
> straight to out and return 0.
> 
> That jump skips all of the following:
> 
>   - ptr_ring_resize()
>   - the queue_index setup
>   - xdp_rxq registration
>   - sock_hold()/tun_napi_init() or tun_enable_queue()
>   - publishing tfile->tun and tun->tfiles[]
>   - the numqueues increment
> 
> tun_set_iff() and tun_set_queue() report success, but the fd stays
> unattached, and later reads and writes fail with -EBADFD.
> 
> The reverse also happens: when the build fails, the code falls through and
> attaches the queue with no filter.
> 
> The next patch in the series, "tun: fix inverted error check when
> re-attaching the filter", changes this to "if (err) goto out;". It also
> adds a rollback_filter sk_detach_filter() on the later failure paths, so
> the code is correct at the end of the series.
> 
> Could that fix be folded into this patch, or ordered before it? Otherwise
> a bisect can land on a kernel where re-attaching to a filtered persistent
> TAP silently does nothing.
> > [ ... ]
> 
>> @@ -3070,7 +3103,7 @@ static int tun_attach_filter(struct tun_struct *tun)
>>  	for (i = 0; i < tun->numqueues; i++) {
>>  		tfile = rtnl_dereference(tun->tfiles[i]);
>>  		lock_sock(tfile->socket.sk);
>> -		ret = sk_attach_filter(&tun->fprog, tfile->socket.sk);
>> +		ret = sk_attach_filter_kern(&tun->fprog_kern, tfile->socket.sk);
>>  		release_sock(tfile->socket.sk);
>>  		if (ret) {
>>  			tun_detach_filter(tun, i);
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but what happens to the
> remaining queues in this case? TUNATTACHFILTER replaces an attached
> filter, and sk_attach_filter_kern() fails on queue i, for example with
> -ENOMEM from __sk_filter_charge() at the optmem limit.
> 
> tun_detach_filter(tun, i) only detaches queues 0..i-1, then frees the new
> kernel copy and clears filter_attached. Queues i..numqueues-1 keep the
> previous sk_filter.
> 
> The previous copy has already been freed in tun_copy_filter():
> 
> 	kfree(tun->fprog_kern.filter);
> 	tun->fprog_kern.len = fprog.len;
> 	tun->fprog_kern.filter = insns;
> 	tun->fprog = fprog;
> 
> So the old state can't be restored, and queues attached later get no
> filter. The device ends up with some queues unfiltered and others still
> using the old filter.
> 
> The old code had the same prefix-only rollback, since it overwrote
> tun->fprog in place before attaching.
> 
> [ ... ]
> 
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 70dc621672f2e..64d6505a4ef23 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -1567,6 +1567,28 @@ int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk)
>>  }
>>  EXPORT_SYMBOL_GPL(sk_attach_filter);
>>  
>> +int sk_attach_filter_kern(struct sock_fprog_kern *fprog, struct sock *sk)
>> +{
>> +	struct bpf_prog *prog;
>> +	int err;
>> +
>> +	if (sock_flag(sk, SOCK_FILTER_LOCKED))
>> +		return -EPERM;
>> +
>> +	err = bpf_prog_create(&prog, fprog);
>> +	if (err)
>> +		return err;
>> +
>> +	err = __sk_attach_prog(prog, sk);
> 
> [Severity: Low]
> bpf_prog_create() is documented as creating an unattached filter, and it
> deliberately skips orig_prog:
> 
> net/core/filter.c:bpf_prog_create() {
> 	...
> 	/* Since unattached filters are not copied back to user
> 	 * space through sk_get_filter(), we do not need to hold
> 	 * a copy here, and can spare us the work.
> 	 */
> 	fp->orig_prog = NULL;
> 	...
> }
> 
> This helper now attaches such a program to sk->sk_filter. Does
> sk_get_filter() then return -EACCES for these sockets?
> 
> net/core/filter.c:sk_get_filter() {
> 	...
> 	ret = -EACCES;
> 	fprog = filter->prog->orig_prog;
> 	if (!fprog)
> 		goto out;
> 	...
> }
> 
> sock_diag would also leave the filter out. sk_attach_filter() keeps
> orig_prog through bpf_prog_store_orig_filter().
> 
> tun sockets can't reach SO_GET_FILTER today. However, this helper is
> exported and declared next to sk_attach_filter() in include/linux/filter.h.
> Should the difference be documented, or should orig_prog be stored here as
> well?
> 
pw-bot: cr