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

[Rongguang Wei <clementwei90@163.com>]

on 2026/10/27 22:05, Willem de Bruijn wrote:
> netdev-bot+sashiko@ wrote:
>> Thank you for your contribution! Sashiko AI review found 2 potential
>> issue(s) to consider.
>>
>> Critical: 0 · High: 1 · Medium: 0 (2) · Low: 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] bpf_prog_validate_classic() does not run "the same checks that
>>   bpf_prog_create() performs", although its kernel-doc and the commit…
>> - [Low] The kernel-doc for the new exported helper
>>   bpf_prog_validate_classic() (net/core/filter.c) says it returns "0 if…
>>
>> Pre-existing issues:
>> - [Medium] This problem wasn't introduced by this patch, but a filter
>>   replacement that passes tun_copy_filter() and then fails on some queue…
>> - [Medium] This problem wasn't introduced by this patch, but
>>   TUNDETACHFILTER leaves disabled queues with their old socket filter,…
>>
>> --
>>
>> tun: keep a kernel copy of the socket filter program
>>
>> This commit keeps a kernel copy of the classic BPF program passed to
>> TUNATTACHFILTER in tun->fprog_kern. Each queue's filter is now built from
>> that copy with a new sk_attach_filter_kern() helper. It also fixes the
>> inverted error check in tun_attach(), and it validates the program up
>> front in tun_copy_filter() with a new bpf_prog_validate_classic() helper.
>>
>> Link: https://lore.kernel.org/netdev/179027275318.2160803.4185895144088175048@kernel.org/
>>
>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>>> index 242899f7fd071..e7d9f4480a89f 100644
>>> --- a/drivers/net/tun.c
>>> +++ b/drivers/net/tun.c
>>
>> [ ... ]
>>
>>> +	/* The instructions are only verified when a program is built from
>>> +	 * them, which tun_attach_filter() does once per attached queue.
>>> +	 * Check them here as well: with no queue attached that would not
>>> +	 * happen at all, and, now that tun_attach() treats that error as
>>> +	 * fatal, every later attach would fail with -EINVAL instead of
>>> +	 * rejecting the program now.
>>> +	 */
>>> +	err = bpf_prog_validate_classic(insns, fprog.len);
>>> +	if (err) {
>>> +		kfree(insns);
>>> +		return err;
>>> +	}
>>
>> [Severity: High]
>> Does bpf_prog_validate_classic() really reject everything that
>> bpf_prog_create() rejects? Its kernel-doc and the commit message both say
>> it does:
>>
>>   "The instructions are verified with the new bpf_prog_validate_classic(),
>>   which runs the same checks as bpf_prog_create() on the copied program."
>>
>> The helper only runs bpf_check_basics_ok() and bpf_check_classic().
>> bpf_prog_create() also goes through this path:
>>
>> bpf_prog_create()
>>   bpf_prepare_filter()
>>     bpf_migrate_filter()
>>       bpf_convert_filter()
>>     bpf_prog_select_runtime()
>>       __bpf_prog_select_runtime()
>>
>> bpf_convert_filter() rejects any jump whose converted offset does not fit
>> in s16:
>>
>> 	if (off < off_min || off > off_max)
>> 		goto err;
>>
>> __bpf_prog_select_runtime() can also return -ENOTSUPP when a JIT is
>> required but did not run.
>>
>> Take a 4096 instruction program: BPF_JMP|BPF_JA with k=4094 at pc 0, then
>> 4094 BPF_LD|BPF_B|BPF_ABS loads of offset 0, then BPF_RET|BPF_K.
>> bpf_check_classic() accepts it. convert_bpf_ld_abs() expands each load
>> into 12 eBPF instructions, so the JA offset grows to about 49128, and
>> bpf_convert_filter() fails with -EINVAL.
>>
>> Suppose every queue on a multiqueue TAP has been disabled with
>> IFF_DETACH_QUEUE. tun_copy_filter() stores this program, and
>> tun_attach_filter() loops over zero queues and then does:
>>
>> 	tun->filter_attached = true;
>>
>> >From then on, every IFF_ATTACH_QUEUE, and every TUNSETIFF without
>> IFF_NOFILTER, calls sk_attach_filter_kern() in tun_attach() and gets
>> -EINVAL. With the corrected check, tun_attach() then takes the goto out
>> path and the queue is never published.
>>
>> Isn't this the "every later attach would fail with -EINVAL" case that the
>> comment above says is avoided?
>>
>> When queues are active, the same program fails at queue 0, and
>> tun_attach_filter() calls tun_detach_filter(tun, 0). That frees fprog_kern
>> and clears filter_attached, which doesn't seem to match this part of the
>> commit message:
>>
>>   "a TUNATTACHFILTER that no verifier would accept now fails early and
>>   leaves the previously attached filter in place."
>>
>> The sk_attach_filter_kern() kernel-doc in this same patch lists -EINVAL
>> from bpf_prog_create() and -ENOTSUPP as possible results. That also
>> suggests the two checks are not equivalent.
>>
>> Would it be more reliable for tun_copy_filter() to build the program once
>> with bpf_prog_create() and then destroy it, instead of calling
>> bpf_prog_validate_classic()?
> 
> This sounds to me as the perfect being the enemy of the good.
> 
> Even the addition of calling bpf_prog_validate_classic is a nice to have.
> This series fixes a real issue.
> 
> Yes, some issues are still only discovered later when the BPF program is
> built. But as long that late discovery is also handled well, that is
> acceptable.
Thanks Willem.

Rather than building the program twice, v8 keeps the cheap check and
commits the copy only after every queue accepted it. A late failure
drops the copies already installed and leaves tun->fprog, fprog_kern and
filter_attached alone. With numqueues == 0 the program is stored and the
first attach fails with -EINVAL.
In selftest, attach_filter_bad_prog_keeps_program() now also checks 
IFF_NOFILTER and a newly attached queue.


pw-bot: cr