[PATCH v2] can: fix skb reference counting in j1939_session_new()

[PATCH v2] can: fix skb reference counting in j1939_session_new()

[Dmitry Antipov]

Since 'j1939_session_skb_queue()' do an extra 'skb_get()' for each
new skb, I assume that the same should be done for an initial one
in 'j1939_session_new()' just to avoid refcount underflow.

Reported-by: syzbot+d4e8dc385d9258220c31@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=d4e8dc385d9258220c31
Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v2: resend after hitting skb refcount underflow once again when looking
around https://syzkaller.appspot.com/bug?extid=0e6ddb1ef80986bdfe64
---
 net/can/j1939/transport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 319f47df3330..95f7a7e65a73 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -1505,7 +1505,7 @@ static struct j1939_session *j1939_session_new(struct j1939_priv *priv,
 	session->state = J1939_SESSION_NEW;
 
 	skb_queue_head_init(&session->skb_queue);
-	skb_queue_tail(&session->skb_queue, skb);
+	skb_queue_tail(&session->skb_queue, skb_get(skb));
 
 	skcb = j1939_skb_to_cb(skb);
 	memcpy(&session->skcb, skcb, sizeof(session->skcb));
-- 
2.47.0

Re: [PATCH v2] can: fix skb reference counting in j1939_session_new()

[Jiri Pirko]

Tue, Nov 05, 2024 at 10:48:23AM CET, dmantipov@yandex.ru wrote:
>Since 'j1939_session_skb_queue()' do an extra 'skb_get()' for each
>new skb, I assume that the same should be done for an initial one

It is odd to write "I assume" for fix like this. You should know for
sure, don't you?


>in 'j1939_session_new()' just to avoid refcount underflow.
>
>Reported-by: syzbot+d4e8dc385d9258220c31@syzkaller.appspotmail.com
>Closes: https://syzkaller.appspot.com/bug?extid=d4e8dc385d9258220c31
>Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
>Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
>---
>v2: resend after hitting skb refcount underflow once again when looking
>around https://syzkaller.appspot.com/bug?extid=0e6ddb1ef80986bdfe64
>---
> net/can/j1939/transport.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
>index 319f47df3330..95f7a7e65a73 100644
>--- a/net/can/j1939/transport.c
>+++ b/net/can/j1939/transport.c
>@@ -1505,7 +1505,7 @@ static struct j1939_session *j1939_session_new(struct j1939_priv *priv,
> 	session->state = J1939_SESSION_NEW;
> 
> 	skb_queue_head_init(&session->skb_queue);
>-	skb_queue_tail(&session->skb_queue, skb);
>+	skb_queue_tail(&session->skb_queue, skb_get(skb));
> 
> 	skcb = j1939_skb_to_cb(skb);
> 	memcpy(&session->skcb, skcb, sizeof(session->skcb));
>-- 
>2.47.0
>
>

Re: [PATCH v2] can: fix skb reference counting in j1939_session_new()

[Oleksij Rempel]

On Tue, Nov 05, 2024 at 05:37:53PM +0100, Jiri Pirko wrote:
> Tue, Nov 05, 2024 at 10:48:23AM CET, dmantipov@yandex.ru wrote:
> >Since 'j1939_session_skb_queue()' do an extra 'skb_get()' for each
> >new skb, I assume that the same should be done for an initial one
> 
> It is odd to write "I assume" for fix like this. You should know for
> sure, don't you?

Hm... looks the there is more then one refcounting problem at this
point. skb_queue is set from 3 different paths, with resulting 3 different
refcount states:

j1939_sk_send_loop()
  skb = j1939_sk_alloc_skb() // skb with refcount == 1
  if (!session) {
    session = j1939_tp_send(priv, skb, size)
       ... 
       session = j1939_session_new(priv, skb, size);
          skb_queue_tail(&session->skb_queue, skb); // skb refcount == 1
          
  } else {
    j1939_session_skb_queue(session, skb);
      // here, skb is refcounted
      skb_queue_tail(&session->skb_queue, skb_get(skb)); // skb refcount == 2
  }
  
  // at the end of function, skb refcount == 1 or 2
     
j1939_xtp_rx_rts_session_new()
  j1939_session_fresh_new()
    skb = alloc_skb() // skb with refcount == 1
    session = j1939_session_new(priv, skb, size);
       skb_queue_tail(&session->skb_queue, skb);
    skb_put(skb, size); // skb with refcount == 0

I agree with this patch, but there is missing skb_put() in j1939_sk_send_loop()

> 
> >in 'j1939_session_new()' just to avoid refcount underflow.
> >
> >Reported-by: syzbot+d4e8dc385d9258220c31@syzkaller.appspotmail.com
> >Closes: https://syzkaller.appspot.com/bug?extid=d4e8dc385d9258220c31
> >Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> >Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> >---
> >v2: resend after hitting skb refcount underflow once again when looking
> >around https://syzkaller.appspot.com/bug?extid=0e6ddb1ef80986bdfe64
> >---
> > net/can/j1939/transport.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
> >index 319f47df3330..95f7a7e65a73 100644
> >--- a/net/can/j1939/transport.c
> >+++ b/net/can/j1939/transport.c
> >@@ -1505,7 +1505,7 @@ static struct j1939_session *j1939_session_new(struct j1939_priv *priv,
> > 	session->state = J1939_SESSION_NEW;
> > 
> > 	skb_queue_head_init(&session->skb_queue);
> >-	skb_queue_tail(&session->skb_queue, skb);
> >+	skb_queue_tail(&session->skb_queue, skb_get(skb));
> > 
> > 	skcb = j1939_skb_to_cb(skb);
> > 	memcpy(&session->skcb, skcb, sizeof(session->skcb));
> >-- 
> >2.47.0
> >
> >
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] can: fix skb reference counting in j1939_session_new()

[Dmitry Antipov]

On 11/5/24 7:37 PM, Jiri Pirko wrote:

> It is odd to write "I assume" for fix like this. You should know for
> sure, don't you?

Well, the final vote is up to the maintainer(s).

Dmitry

Re: [PATCH v2] can: fix skb reference counting in j1939_session_new()

[Dmitry Antipov]

On 11/6/24 12:43 PM, Oleksij Rempel wrote:

> Hm... looks the there is more then one refcounting problem at this
> point. skb_queue is set from 3 different paths, with resulting 3 different
> refcount states:

I'll take a look; anyway I would prefer "one patch per one problem" approach.

Dmitry

Re: [PATCH v2] can: fix skb reference counting in j1939_session_new()

[Jiri Pirko]

Wed, Nov 06, 2024 at 12:03:57PM CET, dmantipov@yandex.ru wrote:
>On 11/5/24 7:37 PM, Jiri Pirko wrote:
>
>> It is odd to write "I assume" for fix like this. You should know for
>> sure, don't you?
>
>Well, the final vote is up to the maintainer(s).

Vote of what?


>
>Dmitry
>

Re: [PATCH v2] can: fix skb reference counting in j1939_session_new()

[Oleksij Rempel]

On Tue, Nov 05, 2024 at 12:48:23PM +0300, Dmitry Antipov wrote:
> Since 'j1939_session_skb_queue()' do an extra 'skb_get()' for each
> new skb, I assume that the same should be done for an initial one
> in 'j1939_session_new()' just to avoid refcount underflow.
> 
> Reported-by: syzbot+d4e8dc385d9258220c31@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=d4e8dc385d9258220c31
> Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>

Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] can: fix skb reference counting in j1939_session_new()

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]

On 29.11.2024 13:55:56, Oleksij Rempel wrote:
> On Tue, Nov 05, 2024 at 12:48:23PM +0300, Dmitry Antipov wrote:
> > Since 'j1939_session_skb_queue()' do an extra 'skb_get()' for each
> > new skb, I assume that the same should be done for an initial one
> > in 'j1939_session_new()' just to avoid refcount underflow.
> > 
> > Reported-by: syzbot+d4e8dc385d9258220c31@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=d4e8dc385d9258220c31
> > Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> 
> Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

Can you re-phrase the commit message. The "assume" is not appropriate :)

Thanks,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] can: fix skb reference counting in j1939_session_new()

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1331 bytes --]

On 29.11.2024 13:59:28, Marc Kleine-Budde wrote:
> On 29.11.2024 13:55:56, Oleksij Rempel wrote:
> > On Tue, Nov 05, 2024 at 12:48:23PM +0300, Dmitry Antipov wrote:
> > > Since 'j1939_session_skb_queue()' do an extra 'skb_get()' for each
> > > new skb, I assume that the same should be done for an initial one
> > > in 'j1939_session_new()' just to avoid refcount underflow.
> > > 
> > > Reported-by: syzbot+d4e8dc385d9258220c31@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=d4e8dc385d9258220c31
> > > Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> > > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> > 
> > Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
> 
> Can you re-phrase the commit message. The "assume" is not appropriate :)

What about:

Since j1939_session_skb_queue() does an extra skb_get() for each new
skb, do the same for the initial one in j1939_session_new() to avoid
refcount underflow.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] can: fix skb reference counting in j1939_session_new()

[Oleksij Rempel]

On Fri, Nov 29, 2024 at 02:05:11PM +0100, Marc Kleine-Budde wrote:
> On 29.11.2024 13:59:28, Marc Kleine-Budde wrote:
> > On 29.11.2024 13:55:56, Oleksij Rempel wrote:
> > > On Tue, Nov 05, 2024 at 12:48:23PM +0300, Dmitry Antipov wrote:
> > > > Since 'j1939_session_skb_queue()' do an extra 'skb_get()' for each
> > > > new skb, I assume that the same should be done for an initial one
> > > > in 'j1939_session_new()' just to avoid refcount underflow.
> > > > 
> > > > Reported-by: syzbot+d4e8dc385d9258220c31@syzkaller.appspotmail.com
> > > > Closes: https://syzkaller.appspot.com/bug?extid=d4e8dc385d9258220c31
> > > > Fixes: 9d71dd0c7009 ("can: add support of SAE J1939 protocol")
> > > > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> > > 
> > > Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > 
> > Can you re-phrase the commit message. The "assume" is not appropriate :)
> 
> What about:
> 
> Since j1939_session_skb_queue() does an extra skb_get() for each new
> skb, do the same for the initial one in j1939_session_new() to avoid
> refcount underflow.

Sounds good. Thx!

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] can: fix skb reference counting in j1939_session_new()

[Oleksij Rempel]

On Wed, Nov 06, 2024 at 10:43:04AM +0100, Oleksij Rempel wrote:
> On Tue, Nov 05, 2024 at 05:37:53PM +0100, Jiri Pirko wrote:
> > Tue, Nov 05, 2024 at 10:48:23AM CET, dmantipov@yandex.ru wrote:
> > >Since 'j1939_session_skb_queue()' do an extra 'skb_get()' for each
> > >new skb, I assume that the same should be done for an initial one
> > 
> > It is odd to write "I assume" for fix like this. You should know for
> > sure, don't you?
> 
> Hm... looks the there is more then one refcounting problem at this
> point. skb_queue is set from 3 different paths, with resulting 3 different
> refcount states:
> 
> j1939_sk_send_loop()
>   skb = j1939_sk_alloc_skb() // skb with refcount == 1
>   if (!session) {
>     session = j1939_tp_send(priv, skb, size)
>        ... 
>        session = j1939_session_new(priv, skb, size);
>           skb_queue_tail(&session->skb_queue, skb); // skb refcount == 1
>           
>   } else {
>     j1939_session_skb_queue(session, skb);
>       // here, skb is refcounted
>       skb_queue_tail(&session->skb_queue, skb_get(skb)); // skb refcount == 2
>   }
>   
>   // at the end of function, skb refcount == 1 or 2
>      
> j1939_xtp_rx_rts_session_new()
>   j1939_session_fresh_new()
>     skb = alloc_skb() // skb with refcount == 1
>     session = j1939_session_new(priv, skb, size);
>        skb_queue_tail(&session->skb_queue, skb);
>     skb_put(skb, size); // skb with refcount == 0
> 
> I agree with this patch, but there is missing skb_put() in j1939_sk_send_loop()

Please forget it, no skb_free is needed in the j1939_sk_send_loop().

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |