diff mbox series

rose: check NULL rose_loopback_neigh->loopback

Message ID 20220816185140.9129-1-bernard.f6bvp@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series rose: check NULL rose_loopback_neigh->loopback | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Bernard Pidoux Aug. 16, 2022, 6:51 p.m. UTC 
From: Bernard <bernard.f6bvp@gmail.com>

Since kernel 5.4.83 rose network connections were no more possible.
Last good rose module was with kernel 5.4.79.

Francois Romieu <romieu@fr.zoreil.com> pointed the scope of changes to
the attached commit (3b3fd068c56e3fbea30090859216a368398e39bf
in mainline, 7f0ddd41e2899349461b578bec18e8bd492e1765 in stable).

Above patch added NULL check for `rose_loopback_neigh->dev`
in rose_loopback_timer() but omitted to check rose_loopback_neigh->loopback.

It thus introduced a bug preventing ALL rose connect.
The reason is that a special rose_neigh loopback has a NULL device.

This is shown in /proc/net/rose_neigh via rose_neigh_show() function :
...
	seq_printf(seq, "%05d %-9s %-4s   %3d %3d  %3s     %3s %3lu %3lu",
	   rose_neigh->number,
	   (rose_neigh->loopback) ? "RSLOOP-0" : ax2asc(buf, &rose_neigh->callsign),
	   rose_neigh->dev ? rose_neigh->dev->name : "???",
	   rose_neigh->count,
...
/proc/net/rose_neigh displays special rose_loopback_neigh->loopback callsign RSLOOP-0:

addr  callsign  dev  count use mode restart  t0  tf digipeaters
00001 RSLOOP-0  ???      1   2  DCE     yes   0   0

By checking rose_loopback_neigh->loopback, rose_rx_call_request() is called even in case 
rose_loopback_neigh->dev is NULL. This repairs rose connections.

Verification with rose client application FPAC:

FPAC-Node v 4.1.3 (built Aug  5 2022) for LINUX (help = h)
F6BVP-4 (Commands = ?) : u
Users - AX.25 Level 2 sessions :
Port   Callsign     Callsign  AX.25 state  ROSE state  NetRom status
axudp  F6BVP-5   -> F6BVP-9   Connected    Connected   ---------

IMHO this patch should be propagated back to LTS 5.4 kernel.

Signed-off-by: Bernard Pidoux <f6bvp@free.fr>
---
 net/rose/rose_loopback.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

--
2.34.1

Comments

Francois Romieu Aug. 16, 2022, 9:57 p.m. UTC | #1 
bernard.f6bvp@gmail.com <bernard.f6bvp@gmail.com> :
> From: Bernard <bernard.f6bvp@gmail.com>
> 
> Since kernel 5.4.83 rose network connections were no more possible.
> Last good rose module was with kernel 5.4.79.
> 
> Francois Romieu <romieu@fr.zoreil.com> pointed the scope of changes to
> the attached commit (3b3fd068c56e3fbea30090859216a368398e39bf
> in mainline, 7f0ddd41e2899349461b578bec18e8bd492e1765 in stable).

The attachment did not follow the references from the original mail. :o/

The paragraph above may be summarized as:

Fixes: 3b3fd068c56e ("rose: Fix Null pointer dereference in rose_send_frame()")

("Suggested-by" would be utter gourmandise)

[...]
> IMHO this patch should be propagated back to LTS 5.4 kernel.

3b3fd068c56e is itself tagged as 'Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")',
i.e. 'problem exists since git epoch back in 2005'. Stable people will
probably apply your fix wherever 3b3fd068c56e has been applied or backported,
namely anything post v5.10, stable v5.4, stable v4.19 and stable v4.14.

> Signed-off-by: Bernard Pidoux <f6bvp@free.fr>
> ---
>  net/rose/rose_loopback.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
> index 11c45c8c6c16..1c673db52636 100644
> --- a/net/rose/rose_loopback.c
> +++ b/net/rose/rose_loopback.c
> @@ -97,8 +97,10 @@ static void rose_loopback_timer(struct timer_list *unused)
> 
> 		if (frametype == ROSE_CALL_REQUEST) {
> 			if (!rose_loopback_neigh->dev) {
> -				kfree_skb(skb);
> -				continue;
> +				if (!rose_loopback_neigh->loopback) {
> +					kfree_skb(skb);
> +					continue;
> +				}

FWIW, avoiding the extra indentation may be marginally more idiomatic:

@@ -96,7 +96,8 @@ static void rose_loopback_timer(struct timer_list *unused)
		}

		if (frametype == ROSE_CALL_REQUEST) {
-			if (!rose_loopback_neigh->dev) {
+			if (!rose_loopback_neigh->dev &&
+			    !rose_loopback_neigh->loopback) {
 				kfree_skb(skb);
 				continue;
			}
Good night.
Bernard Pidoux Aug. 17, 2022, 9:20 a.m. UTC | #2 
Hi,

I absolutely agree with all your remarks, suggestions and nice 
improvement to my patch.

As I am definitively an amateur and not familiar with git send-email, 
may I ask you to resubmit the modified patch for me including:

Suggested-by Francois Romieu <romieu@fr.zoreil.com>

Thanks a lot.

Bernard

Le 16/08/2022 à 23:57, Francois Romieu a écrit :
> bernard.f6bvp@gmail.com <bernard.f6bvp@gmail.com> :
>> From: Bernard <bernard.f6bvp@gmail.com>
>>
>> Since kernel 5.4.83 rose network connections were no more possible.
>> Last good rose module was with kernel 5.4.79.
>>
>> Francois Romieu <romieu@fr.zoreil.com> pointed the scope of changes to
>> the attached commit (3b3fd068c56e3fbea30090859216a368398e39bf
>> in mainline, 7f0ddd41e2899349461b578bec18e8bd492e1765 in stable).
> 
> The attachment did not follow the references from the original mail. :o/
> 
> The paragraph above may be summarized as:
> 
> Fixes: 3b3fd068c56e ("rose: Fix Null pointer dereference in rose_send_frame()")
> 
> ("Suggested-by" would be utter gourmandise)
> 
> [...]
>> IMHO this patch should be propagated back to LTS 5.4 kernel.
> 
> 3b3fd068c56e is itself tagged as 'Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")',
> i.e. 'problem exists since git epoch back in 2005'. Stable people will
> probably apply your fix wherever 3b3fd068c56e has been applied or backported,
> namely anything post v5.10, stable v5.4, stable v4.19 and stable v4.14.
> 
>> Signed-off-by: Bernard Pidoux <f6bvp@free.fr>
>> ---
>>   net/rose/rose_loopback.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
>> index 11c45c8c6c16..1c673db52636 100644
>> --- a/net/rose/rose_loopback.c
>> +++ b/net/rose/rose_loopback.c
>> @@ -97,8 +97,10 @@ static void rose_loopback_timer(struct timer_list *unused)
>>
>> 		if (frametype == ROSE_CALL_REQUEST) {
>> 			if (!rose_loopback_neigh->dev) {
>> -				kfree_skb(skb);
>> -				continue;
>> +				if (!rose_loopback_neigh->loopback) {
>> +					kfree_skb(skb);
>> +					continue;
>> +				}
> 
> FWIW, avoiding the extra indentation may be marginally more idiomatic:
> 
> @@ -96,7 +96,8 @@ static void rose_loopback_timer(struct timer_list *unused)
> 		}
> 
> 		if (frametype == ROSE_CALL_REQUEST) {
> -			if (!rose_loopback_neigh->dev) {
> +			if (!rose_loopback_neigh->dev &&
> +			    !rose_loopback_neigh->loopback) {
>   				kfree_skb(skb);
>   				continue;
> 			}
> Good night.
>
diff mbox series

Patch

diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
index 11c45c8c6c16..1c673db52636 100644
--- a/net/rose/rose_loopback.c
+++ b/net/rose/rose_loopback.c
@@ -97,8 +97,10 @@  static void rose_loopback_timer(struct timer_list *unused)

		if (frametype == ROSE_CALL_REQUEST) {
			if (!rose_loopback_neigh->dev) {
-				kfree_skb(skb);
-				continue;
+				if (!rose_loopback_neigh->loopback) {
+					kfree_skb(skb);
+					continue;
+				}
			}

			dev = rose_dev_get(dest);