On Mon, May 22 2023, SeongJae Park wrote:
> Hi Pratyush,
>
> On Mon, 22 May 2023 17:30:20 +0200 Pratyush Yadav <ptyadav(a)amazon.de> wrote:
>
>> Commit 50749f2dd685 ("tcp/udp: Fix memleaks of sk and zerocopy skbs with
>> TX timestamp.") added a call to skb_orphan_frags_rx() to fix leaks with
>> zerocopy skbs. But it ended up adding a leak of its own. When
>> skb_orphan_frags_rx() fails, the function just returns, leaking the skb
>> it just cloned. Free it before returning.
>>
>> This bug was discovered and resolved using Coverity Static Analysis
>> Security Testing (SAST) by Synopsys, Inc.
>>
>> Fixes: 50749f2dd685 ("tcp/udp: Fix memleaks of sk and zerocopy skbs with TX timestamp.")
>
> Seems the commit has merged in several stable kernels. Is the bug also
> affecting those? If so, would it be better to Cc stable(a)vger.kernel.org?
>
It affects v5.4.243 at least, since that is where I first saw this. But
I would expect it to affect other stable kernels it has been backported
to as well. I thought using the Fixes tag pointing to the bad upstream
commit would be enough for the stable maintainers' tooling/bots to pick
this patch up.
In either case, +Cc stable. Link to the patch this thread is talking
about [0].
[0] https://lore.kernel.org/netdev/20230522153020.32422-1-ptyadav@amazon.de/T/#u
>
>
> Thanks,
> SJ
>
>> Signed-off-by: Pratyush Yadav <ptyadav(a)amazon.de>
>> ---
>>
>> I do not know this code very well, this was caught by our static
>> analysis tool. I did not try specifically reproducing the leak but I did
>> do a boot test by adding this patch on 6.4-rc3 and the kernel boots
>> fine.
>>
>> net/core/skbuff.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index 515ec5cdc79c..cea28d30abb5 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -5224,8 +5224,10 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
>> } else {
>> skb = skb_clone(orig_skb, GFP_ATOMIC);
>>
>> - if (skb_orphan_frags_rx(skb, GFP_ATOMIC))
>> + if (skb_orphan_frags_rx(skb, GFP_ATOMIC)) {
>> + kfree_skb(skb);
>> return;
>> + }
>> }
>> if (!skb)
>> return;
>> --
>> 2.39.2
>>
--
Regards,
Pratyush Yadav
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
[Adding a few pople to the list of recipients that were involved in
developing the culprit; also CCing the regression list, as it should be
in the loop for regressions:
https://docs.kernel.org/admin-guide/reporting-regressions.html]
[TLDR: I'm adding this report to the list of tracked Linux kernel
regressions; the text you find below is based on a few templates
paragraphs you might have encountered already in similar form.
See link in footer if these mails annoy you.]
On 29.03.23 16:31, Kristof Havasi wrote:
>
> I was rebasing the Kernel branch of our SAMA5D35 based board from
> v5.4.189 to v5.4.238.
> I noticed that after the rebase we could _only send, but not receive_
> through our RS485 interface.
>
> I could bisect the problem to 77b97ef4908aa917e7b68667ec6b344cc5dc5034
> in the v5.4.225 release.
FWIW, that's 7176a6a8982d ("dmaengine: at_hdmac: Don't start
transactions at tx_submit level") in mainline.
Kristof Havasi: would be good to know if this is something that happens
with recent mainline as well, because if not it might be something the
stable team needs to handle.
> If I revert this commit, the tx/rx works just
> like before.
> Maybe this use-case wasn't considered when this patch was created?
> I haven't seen a documentation change regarding this in DT bindings,
> but if the config should be something else, please let me know.
> Otherwise this commit breaks the RS485 function of atmel_serial at
> least in the v5.4.y branch.
>
> Best Regards,
> Kristóf Havasi
>
> The relevant device tree nodes:
>
> from sama5d3.dtsi:
>
> usart1: serial@f0020000 {
> compatible = "atmel,at91sam9260-usart";
> reg = <0xf0020000 0x100>;
> interrupts = <13 IRQ_TYPE_LEVEL_HIGH 5>;
> dmas = <&dma0 2 AT91_DMA_CFG_PER_ID(5)>,
> <&dma0 2 (AT91_DMA_CFG_PER_ID(6) | AT91_DMA_CFG_FIFOCFG_ASAP)>;
> dma-names = "tx", "rx";
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_usart1>;
> clocks = <&usart1_clk>;
> clock-names = "usart";
> status = "disabled";
> };
>
> pinctrl_usart1: usart1-0 {
> atmel,pins =
> <AT91_PIOB 28 AT91_PERIPH_A AT91_PINCTRL_PULL_UP
> AT91_PIOB 29 AT91_PERIPH_A AT91_PINCTRL_NONE>;
> };
> pinctrl_usart1_rts_cts: usart1_rts_cts-0 {
> atmel,pins =
> <AT91_PIOB 26 AT91_PERIPH_A AT91_PINCTRL_NONE /* PB26 periph A,
> conflicts with GRX7 */
> AT91_PIOB 27 AT91_PERIPH_A AT91_PINCTRL_NONE>; /* PB27 periph A,
> conflicts with G125CKO */
> };
>
> from our dts:
>
> &usart1 {
> pinctrl-0 = <&pinctrl_usart1 &pinctrl_usart1_rts_cts>;
> atmel,use-dma-rx;
> atmel,use-dma-tx;
> rs485-rx-during-tx;
> linux,rs485-enabled-at-boot-time;
> status = "okay";
> };
>
> HW:
> The SAMA5D3's PB27 is connected to the |RE+DE of the RS485 transceiver
> SP3458EN-L
Thanks for the report. To be sure the issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
tracking bot:
#regzbot ^introduced 77b97ef4908aa
#regzbot title dmaengine: at_hdmac: receiving data through the RS485
interface broke
#regzbot ignore-activity
This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply and tell me -- ideally
while also telling regzbot about it, as explained by the page listed in
the footer of this mail.
Developers: When fixing the issue, remember to add 'Link:' tags pointing
to the report (the parent of this mail). See page linked in footer for
details.
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.