From mboxrd@z Thu Jan  1 00:00:00 1970
From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Subject: Re: [PATCH net] tun: Fix NULL pointer dereference in XDP redirect
Date: Mon, 28 May 2018 13:28:33 +0900
Message-ID: <ba61e92b-cfd8-8e5c-d1db-1cf598dfb655@lab.ntt.co.jp>
References: <1527222729-2561-1-git-send-email-makita.toshiaki@lab.ntt.co.jp>
 <491862dc-b6d2-854d-4d5f-8dfe34da882d@redhat.com>
 <863681d4-92cf-1575-a182-b07f22ec578e@gmail.com>
 <e7222fda-a845-e1ac-f7c2-9a019951af12@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Cc: Toshiaki Makita <toshiaki.makita1@gmail.com>,
        "David S. Miller" <davem@davemloft.net>, netdev@vger.kernel.org,
        "Michael S. Tsirkin" <mst@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from tama500.ecl.ntt.co.jp ([129.60.39.148]:47039 "EHLO
        tama500.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1750710AbeE1E3X (ORCPT
        <rfc822;netdev@vger.kernel.org>); Mon, 28 May 2018 00:29:23 -0400
In-Reply-To: <e7222fda-a845-e1ac-f7c2-9a019951af12@redhat.com>
Content-Language: en-US
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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