On 2018/05/28 11:24, Jason Wang wrote:
> On 2018年05月25日 21:43, Toshiaki Makita wrote:
>
> [...]
>
>>>> @@ -1917,16 +1923,22 @@ static ssize_t tun_get_user(struct
>>>> tun_struct *tun, struct tun_file *tfile,
>>>> struct bpf_prog *xdp_prog;
>>>> int ret;
>>>> + local_bh_disable();
>>>> + preempt_disable();
>>>> rcu_read_lock();
>>>> xdp_prog = rcu_dereference(tun->xdp_prog);
>>>> if (xdp_prog) {
>>>> ret = do_xdp_generic(xdp_prog, skb);
>>>> if (ret != XDP_PASS) {
>>>> rcu_read_unlock();
>>>> + preempt_enable();
>>>> + local_bh_enable();
>>>> return total_len;
>>>> }
>>>> }
>>>> rcu_read_unlock();
>>>> + preempt_enable();
>>>> + local_bh_enable();
>>>> }
>>>> rcu_read_lock();
>>>
>>> Good catch, thanks.
>>>
>>> But I think we can just replace preempt_disable()/enable() with
>>> local_bh_disable()/local_bh_enable() ?
>>
>> I actually thought the same, but noticed this patch.
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9ea4c380066fbe
>>
>>
>> It looks like they do not think local_bh_disable() implies
>> preempt_disable(). But I'm not sure why..
>>
>> Toshiaki Makita
>
> I see, there're probably have some subtle differences and implications
> for e.g scheduler or others.
>
> What we what here is to make sure the process is not moved to another
> CPU and bh is enabled. By checking preemptible() function, preemption
> should be disabled after local_bh_disable(). So I think we're safe here.
OK. I checked retint_kernel which IIUC is the entry point of preemption
process on x86, and confirmed it just checks if __preempt_count is zero.
I haven't checked other archs but I was probably worried too much.
Will send v2.
Thanks,
Toshiaki Makita
