On 13/06/2024 21:27, Ken Milmore wrote:
On 13/06/2024 18:21, Alexey Khoroshilov wrote:
On 13.06.2024 14:30, Greg Kroah-Hartman wrote:
5.10-stable review patch. If anyone has any objections, please let me know.
The patch is cleanly applied to 5.10, but it leads to uninit value access in rtl_tx_slots_avail().
unsigned int frags; u32 opts[2];
txd_first = tp->TxDescArray + entry;
if (unlikely(!rtl_tx_slots_avail(tp, frags))) { ^^^^^ - USE OF UNINIT VALUE if (net_ratelimit()) netdev_err(dev, "BUG! Tx Ring full when queue awake!\n"); goto err_stop_0; }
opts[1] = rtl8169_tx_vlan_tag(skb); opts[0] = 0;
if (!rtl_chip_supports_csum_v2(tp)) rtl8169_tso_csum_v1(skb, opts); else if (!rtl8169_tso_csum_v2(tp, skb, opts)) goto err_dma_0;
if (unlikely(rtl8169_tx_map(tp, opts, skb_headlen(skb), skb->data, entry, false))) goto err_dma_0;
txd_first = tp->TxDescArray + entry;
frags = skb_shinfo(skb)->nr_frags; ^^^^^^ - INITIALIZATION IS HERE AFTER THE PATCH
There is no such problem in upstream because rtl_tx_slots_avail() has no nr_frags argument there.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
-- Alexey Khoroshilov Linux Verification Center, ISPRAS
Looks like the frags argument was removed in commit 83c317d7b36bb (r8169: remove nr_frags argument from rtl_tx_slots_avail), which first appears in linux-5.11.
I dare say it would be safe to replace if (unlikely(!rtl_tx_slots_avail(tp, frags))) { with if (unlikely(!rtl_tx_slots_avail(tp, MAX_SKB_FRAGS))) {
Best wait for Heiner to confirm though.
Further to this, I have tested the following amended patch to 5.10 on the RTL8125b and it works without problems:
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index c29d43c5f450..8aa0097bf887 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -4279,16 +4279,16 @@ static void rtl8169_doorbell(struct rtl8169_private *tp) static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb, struct net_device *dev) { - unsigned int frags = skb_shinfo(skb)->nr_frags; struct rtl8169_private *tp = netdev_priv(dev); unsigned int entry = tp->cur_tx % NUM_TX_DESC; struct TxDesc *txd_first, *txd_last; bool stop_queue, door_bell; + unsigned int frags; u32 opts[2];
txd_first = tp->TxDescArray + entry;
- if (unlikely(!rtl_tx_slots_avail(tp, frags))) { + if (unlikely(!rtl_tx_slots_avail(tp, MAX_SKB_FRAGS))) { if (net_ratelimit()) netdev_err(dev, "BUG! Tx Ring full when queue awake!\n"); goto err_stop_0; @@ -4309,6 +4309,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb, entry, false))) goto err_dma_0;
+ frags = skb_shinfo(skb)->nr_frags; if (frags) { if (rtl8169_xmit_frags(tp, skb, opts, entry)) goto err_dma_1;
An alternative to this amendment would be to apply 83c317d7b36bb as a prerequisite, but that does not apply cleanly to 5.10 without some massaging.
Because of these complications, I would suggest it may be safest to omit this patch from 5.10 for now.
NAKed-by: Ken Milmore ken.milmore@gmail.com