On Thu, 5 Nov 2020 11:31:48 +0100 Heiner Kallweit wrote:
> >> @@ -4125,7 +4133,7 @@ static bool rtl8169_tso_csum_v2(struct
> >> rtl8169_private *tp,
> >>
> >> opts[1] |= transport_offset << TCPHO_SHIFT;
> >> } else {
> >> - if (unlikely(rtl_test_hw_pad_bug(tp, skb)))
> >> + if (unlikely(skb->len < ETH_ZLEN && rtl_test_hw_pad_bug(tp)))
> >> return !eth_skb_pad(skb);
> >> }
> >
> > But looks like we may have another bug here - looks like this function
> > treas skb_cow_head() and eth_skb_pad() failures the same, but former
> > doesn't free the skb on error, while the latter does.
> >
> Thanks for the hint, indeed we have an issue. The caller of
> rtl8169_tso_csum_v2() also frees the skb if false is returned, therefore
> we have a double free if eth_skb_pad() fails.
>
> When checking eth_skb_pad() I saw that it uses kfree_skb() to free
> the skb on error. Kernel documentation say about ndo_start_xmit context:
>
> Process with BHs disabled or BH (timer),
> will be called with interrupts disabled by netconsole.
>
> Is it safe to use kfree_skb() if interrupts are disabled?
> I'm asking because dev_kfree_skb_any() uses the irq path if
> irqs_disabled() is true.
It is not, although in practice it only matters if skb has a socket
attached, and AFAIR netconsole doesn't, so no-one has ever seen the
WARN() trigger. But yes, I think it's best if we fix it.
