Re: [PATCH net] tun: Fix NULL pointer dereference in XDP redirect

[Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>]

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