From: Jia-Ju Bai <baijiaju1990@gmail.com>
To: Yanjun Zhu <yanjun.zhu@oracle.com>,
davem@davemloft.net, keescook@chromium.org
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net: nvidia: forcedeth: Fix two possible concurrency use-after-free bugs
Date: Wed, 9 Jan 2019 11:20:52 +0800 [thread overview]
Message-ID: <54c2efce-18ac-46d4-7fb5-8510699b528e@gmail.com> (raw)
In-Reply-To: <a861dacb-c992-13d7-5458-09445183655b@oracle.com>
On 2019/1/9 10:35, Yanjun Zhu wrote:
>
> On 2019/1/9 10:03, Jia-Ju Bai wrote:
>>
>>
>> On 2019/1/9 9:24, Yanjun Zhu wrote:
>>>
>>> On 2019/1/8 20:57, Jia-Ju Bai wrote:
>>>>
>>>>
>>>> On 2019/1/8 20:54, Zhu Yanjun wrote:
>>>>>
>>>>> 在 2019/1/8 20:45, Jia-Ju Bai 写道:
>>>>>> In drivers/net/ethernet/nvidia/forcedeth.c, the functions
>>>>>> nv_start_xmit() and nv_start_xmit_optimized() can be concurrently
>>>>>> executed with nv_poll_controller().
>>>>>>
>>>>>> nv_start_xmit
>>>>>> line 2321: prev_tx_ctx->skb = skb;
>>>>>>
>>>>>> nv_start_xmit_optimized
>>>>>> line 2479: prev_tx_ctx->skb = skb;
>>>>>>
>>>>>> nv_poll_controller
>>>>>> nv_do_nic_poll
>>>>>> line 4134: spin_lock(&np->lock);
>>>>>> nv_drain_rxtx
>>>>>> nv_drain_tx
>>>>>> nv_release_txskb
>>>>>> line 2004: dev_kfree_skb_any(tx_skb->skb);
>>>>>>
>>>>>> Thus, two possible concurrency use-after-free bugs may occur.
>>>>>>
>>>>>> To fix these possible bugs,
>>>>>
>>>>>
>>>>> Does this really occur? Can you reproduce this ?
>>>>
>>>> This bug is not found by the real execution.
>>>> It is found by a static tool written by myself, and then I check it
>>>> by manual code review.
>>>
>>> Before "line 2004: dev_kfree_skb_any(tx_skb->skb); ",
>>>
>>> "
>>>
>>> nv_disable_irq(dev);
>>> nv_napi_disable(dev);
>>> netif_tx_lock_bh(dev);
>>> netif_addr_lock(dev);
>>> spin_lock(&np->lock);
>>> /* stop engines */
>>> nv_stop_rxtx(dev); <---this stop rxtx
>>> nv_txrx_reset(dev);
>>> "
>>>
>>> In this case, does nv_start_xmit or nv_start_xmit_optimized still
>>> work well?
>>>
>>
>> nv_stop_rxtx() calls nv_stop_tx(dev).
>>
>> static void nv_stop_tx(struct net_device *dev)
>> {
>> struct fe_priv *np = netdev_priv(dev);
>> u8 __iomem *base = get_hwbase(dev);
>> u32 tx_ctrl = readl(base + NvRegTransmitterControl);
>>
>> if (!np->mac_in_use)
>> tx_ctrl &= ~NVREG_XMITCTL_START;
>> else
>> tx_ctrl |= NVREG_XMITCTL_TX_PATH_EN;
>> writel(tx_ctrl, base + NvRegTransmitterControl);
>> if (reg_delay(dev, NvRegTransmitterStatus, NVREG_XMITSTAT_BUSY, 0,
>> NV_TXSTOP_DELAY1, NV_TXSTOP_DELAY1MAX))
>> netdev_info(dev, "%s: TransmitterStatus remained busy\n",
>> __func__);
>>
>> udelay(NV_TXSTOP_DELAY2);
>> if (!np->mac_in_use)
>> writel(readl(base + NvRegTransmitPoll) &
>> NVREG_TRANSMITPOLL_MAC_ADDR_REV,
>> base + NvRegTransmitPoll);
>> }
>>
>> nv_stop_tx() seems to only write registers to stop transmitting for
>> hardware.
>> But it does not wait until nv_start_xmit() and
>> nv_start_xmit_optimized() finish execution.
> There are 3 modes in forcedeth NIC.
> In throughput mode (0), every tx & rx packet will generate an interrupt.
> In CPU mode (1), interrupts are controlled by a timer.
> In dynamic mode (2), the mode toggles between throughput and CPU mode
> based on network load.
>
> From the source code,
>
> "np->recover_error = 1;" is related with CPU mode.
>
> nv_start_xmit or nv_start_xmit_optimized seems related with ghroughput
> mode.
>
> In static void nv_do_nic_poll(struct timer_list *t),
> when if (np->recover_error), line 2004:
> dev_kfree_skb_any(tx_skb->skb); will run.
>
> When "np->recover_error=1", do you think nv_start_xmit or
> nv_start_xmit_optimized will be called?
Sorry, I do not know about these modes...
But I still think nv_start_xmit() or nv_start_xmit_optimized() can be
called here, in no matter which mode :)
Best wishes,
Jia-Ju Bai
next prev parent reply other threads:[~2019-01-09 3:21 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-08 12:45 [PATCH] net: nvidia: forcedeth: Fix two possible concurrency use-after-free bugs Jia-Ju Bai
2019-01-08 12:54 ` Zhu Yanjun
2019-01-08 12:57 ` Jia-Ju Bai
2019-01-09 1:24 ` Yanjun Zhu
2019-01-09 2:03 ` Jia-Ju Bai
2019-01-09 2:35 ` Yanjun Zhu
2019-01-09 3:20 ` Jia-Ju Bai [this message]
2019-01-09 3:24 ` Yanjun Zhu
2019-01-09 3:25 ` Jia-Ju Bai
2019-01-12 1:53 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54c2efce-18ac-46d4-7fb5-8510699b528e@gmail.com \
--to=baijiaju1990@gmail.com \
--cc=davem@davemloft.net \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=yanjun.zhu@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).