| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e4f6b9e1-d51a-49f9-9c75-57ef1ea7babf@linux.dev>
Date: Wed, 30 Apr 2025 17:47:13 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Taehee Yoo <ap420073@...il.com>
Cc: Michael Chan <michael.chan@...adcom.com>,
Pavan Chebbi <pavan.chebbi@...adcom.com>, Jakub Kicinski <kuba@...nel.org>,
Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net v4] bnxt_en: improve TX timestamping FIFO
configuration
On 30/04/2025 17:43, Taehee Yoo wrote:
> On Wed, Apr 30, 2025 at 11:55 PM Vadim Fedorenko
> <vadim.fedorenko@...ux.dev> wrote:
>>
>> On 30/04/2025 13:59, Taehee Yoo wrote:
>>> On Thu, Apr 24, 2025 at 10:11 PM Vadim Fedorenko <vadfed@...a.com> wrote:
>>>>
>>>
>>> Hi Vadim,
>>> Thanks for this work!
>>>
>>>> Reconfiguration of netdev may trigger close/open procedure which can
>>>> break FIFO status by adjusting the amount of empty slots for TX
>>>> timestamps. But it is not really needed because timestamps for the
>>>> packets sent over the wire still can be retrieved. On the other side,
>>>> during netdev close procedure any skbs waiting for TX timestamps can be
>>>> leaked because there is no cleaning procedure called. Free skbs waiting
>>>> for TX timestamps when closing netdev.
>>>>
>>>> Fixes: 8aa2a79e9b95 ("bnxt_en: Increase the max total outstanding PTP TX packets to 4")
>>>> Reviewed-by: Michael Chan <michael.chan@...adcom.com>
>>>> Reviewed-by: Pavan Chebbi <pavan.chebbi@...adcom.com>
>>>> Signed-off-by: Vadim Fedorenko <vadfed@...a.com>
>>>> ---
>>>> v3 -> v4:
>>>> * actually remove leftover unused variable in bnxt_ptp_clear()
>>>> (v3 was not committed before preparing unfortunately)
>>>> v2 -> v3:
>>>> * remove leftover unused variable in bnxt_ptp_clear()
>>>> v1 -> v2:
>>>> * move clearing of TS skbs to bnxt_free_tx_skbs
>>>> * remove spinlock as no TX is possible after bnxt_tx_disable()
>>>> * remove extra FIFO clearing in bnxt_ptp_clear()
>>>> ---
>>>> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 5 ++--
>>>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 29 ++++++++++++++-----
>>>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h | 1 +
>>>> 3 files changed, 25 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>>>> index c8e3468eee61..2c8e2c19d854 100644
>>>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>>>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>>>> @@ -3414,6 +3414,9 @@ static void bnxt_free_tx_skbs(struct bnxt *bp)
>>>>
>>>> bnxt_free_one_tx_ring_skbs(bp, txr, i);
>>>> }
>>>> +
>>>> + if (bp->ptp_cfg && !(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
>>>> + bnxt_ptp_free_txts_skbs(bp->ptp_cfg);
>>>> }
>>>>
>>>> static void bnxt_free_one_rx_ring(struct bnxt *bp, struct bnxt_rx_ring_info *rxr)
>>>> @@ -12797,8 +12800,6 @@ static int __bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
>>>> /* VF-reps may need to be re-opened after the PF is re-opened */
>>>> if (BNXT_PF(bp))
>>>> bnxt_vf_reps_open(bp);
>>>> - if (bp->ptp_cfg && !(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
>>>> - WRITE_ONCE(bp->ptp_cfg->tx_avail, BNXT_MAX_TX_TS);
>>>> bnxt_ptp_init_rtc(bp, true);
>>>> bnxt_ptp_cfg_tstamp_filters(bp);
>>>> if (BNXT_SUPPORTS_MULTI_RSS_CTX(bp))
>>>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>>>> index 2d4e19b96ee7..0669d43472f5 100644
>>>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>>>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>>>> @@ -794,6 +794,27 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
>>>> return HZ;
>>>> }
>>>>
>>>> +void bnxt_ptp_free_txts_skbs(struct bnxt_ptp_cfg *ptp)
>>>> +{
>>>> + struct bnxt_ptp_tx_req *txts_req;
>>>> + u16 cons = ptp->txts_cons;
>>>> +
>>>> + /* make sure ptp aux worker finished with
>>>> + * possible BNXT_STATE_OPEN set
>>>> + */
>>>> + ptp_cancel_worker_sync(ptp->ptp_clock);
>>>> +
>>>> + ptp->tx_avail = BNXT_MAX_TX_TS;
>>>> + while (cons != ptp->txts_prod) {
>>>> + txts_req = &ptp->txts_req[cons];
>>>> + if (!IS_ERR_OR_NULL(txts_req->tx_skb))
>>>> + dev_kfree_skb_any(txts_req->tx_skb);
>>>> + cons = NEXT_TXTS(cons);
>>>> + }
>>>> + ptp->txts_cons = cons;
>>>> + ptp_schedule_worker(ptp->ptp_clock, 0);
>>>> +}
>>>> +
>>>> int bnxt_ptp_get_txts_prod(struct bnxt_ptp_cfg *ptp, u16 *prod)
>>>> {
>>>> spin_lock_bh(&ptp->ptp_tx_lock);
>>>> @@ -1105,7 +1126,6 @@ int bnxt_ptp_init(struct bnxt *bp)
>>>> void bnxt_ptp_clear(struct bnxt *bp)
>>>> {
>>>> struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
>>>> - int i;
>>>>
>>>> if (!ptp)
>>>> return;
>>>> @@ -1117,12 +1137,5 @@ void bnxt_ptp_clear(struct bnxt *bp)
>>>> kfree(ptp->ptp_info.pin_config);
>>>> ptp->ptp_info.pin_config = NULL;
>>>>
>>>> - for (i = 0; i < BNXT_MAX_TX_TS; i++) {
>>>> - if (ptp->txts_req[i].tx_skb) {
>>>> - dev_kfree_skb_any(ptp->txts_req[i].tx_skb);
>>>> - ptp->txts_req[i].tx_skb = NULL;
>>>> - }
>>>> - }
>>>> -
>>>> bnxt_unmap_ptp_regs(bp);
>>>> }
>>>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
>>>> index a95f05e9c579..0481161d26ef 100644
>>>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
>>>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
>>>> @@ -162,6 +162,7 @@ int bnxt_ptp_cfg_tstamp_filters(struct bnxt *bp);
>>>> void bnxt_ptp_reapply_pps(struct bnxt *bp);
>>>> int bnxt_hwtstamp_set(struct net_device *dev, struct ifreq *ifr);
>>>> int bnxt_hwtstamp_get(struct net_device *dev, struct ifreq *ifr);
>>>> +void bnxt_ptp_free_txts_skbs(struct bnxt_ptp_cfg *ptp);
>>>> int bnxt_ptp_get_txts_prod(struct bnxt_ptp_cfg *ptp, u16 *prod);
>>>> void bnxt_get_tx_ts_p5(struct bnxt *bp, struct sk_buff *skb, u16 prod);
>>>> int bnxt_get_rx_ts_p5(struct bnxt *bp, u64 *ts, u32 pkt_ts);
>>>> --
>>>> 2.47.1
>>>>
>>>>
>>>
>>> I’ve encountered a kernel panic that I think is related to this patch.
>>> Could you please investigate it?
>>>
>>> Reproducer:
>>> ip link set $interface up
>>> modprobe -rv bnxt_en
>>>
>>
>> Hi Taehee!
>>
>> Yeah, looks like there are some issues on the remove path.
>> Could you please test the diff which may fix the problem:
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> index 78e496b0ec26..86a5de44b6f3 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> @@ -16006,8 +16006,8 @@ static void bnxt_remove_one(struct pci_dev *pdev)
>>
>> bnxt_rdma_aux_device_del(bp);
>>
>> - bnxt_ptp_clear(bp);
>> unregister_netdev(dev);
>> + bnxt_ptp_clear(bp);
>>
>> bnxt_rdma_aux_device_uninit(bp);
>>
>
> Thanks! It seems to fix the issue.
> The above reproducer can't reproduce a kernel panic.
Great, thanks, I'll post a proper patch soon
Powered by blists - more mailing lists