[PATCH net 1/1] net: ppp: fix creating PPP pass and active filters

[PATCH net 1/1] net: ppp: fix creating PPP pass and active filters

[Christoph Schulz]

From: Christoph Schulz <develop@kristov.de>

Commit 568f194e8bd16c353ad50f9ab95d98b20578a39d ("net: ppp: use
sk_unattached_filter api") inadvertently changed the logic when setting
PPP pass and active filters. This applies to both the generic PPP subsystem
implemented by drivers/net/ppp/ppp_generic.c and the ISDN PPP subsystem
implemented by drivers/isdn/i4l/isdn_ppp.c. The original code in ppp_ioctl()
(or isdn_ppp_ioctl(), resp.) handling PPPIOCSPASS and PPPIOCSACTIVE allowed to
remove a pass/active filter previously set by using a filter of length zero.
However, with the new code this is not possible anymore as this case is not
explicitly checked for, which leads to passing NULL as a filter to
sk_unattached_filter_create(). This results in returning EINVAL to the caller.

Additionally, the variables ppp->pass_filter and ppp->active_filter (or
is->pass_filter and is->active_filter, resp.) are not reset to NULL, although
the filters they point to may have been destroyed by
sk_unattached_filter_destroy(), so in this EINVAL case dangling pointers are
left behind (provided the pointers were previously non-NULL).

This patch corrects both problems by checking whether the filter passed is
empty or non-empty, and prevents sk_unattached_filter_create() from being
called in the first case. Moreover, the pointers are always reset to NULL
as soon as sk_unattached_filter_destroy() returns.

Signed-off-by: Christoph Schulz <develop@kristov.de>
---
diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c
index 61ac632..cd2f4c3 100644
--- a/drivers/isdn/i4l/isdn_ppp.c
+++ b/drivers/isdn/i4l/isdn_ppp.c
@@ -644,9 +644,15 @@ isdn_ppp_ioctl(int min, struct file *file, unsigned int cmd, unsigned long arg)
 		fprog.len = len;
 		fprog.filter = code;
 
-		if (is->pass_filter)
+		if (is->pass_filter) {
 			sk_unattached_filter_destroy(is->pass_filter);
-		err = sk_unattached_filter_create(&is->pass_filter, &fprog);
+			is->pass_filter = NULL;
+		}
+		if (fprog.filter != NULL)
+			err = sk_unattached_filter_create(&is->pass_filter,
+							  &fprog);
+		else
+			err = 0;
 		kfree(code);
 
 		return err;
@@ -663,9 +669,15 @@ isdn_ppp_ioctl(int min, struct file *file, unsigned int cmd, unsigned long arg)
 		fprog.len = len;
 		fprog.filter = code;
 
-		if (is->active_filter)
+		if (is->active_filter) {
 			sk_unattached_filter_destroy(is->active_filter);
-		err = sk_unattached_filter_create(&is->active_filter, &fprog);
+			is->active_filter = NULL;
+		}
+		if (fprog.filter != NULL)
+			err = sk_unattached_filter_create(&is->active_filter,
+							  &fprog);
+		else
+			err = 0;
 		kfree(code);
 
 		return err;
diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 91d6c12..d0f6f93 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -763,10 +763,15 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			};
 
 			ppp_lock(ppp);
-			if (ppp->pass_filter)
+			if (ppp->pass_filter) {
 				sk_unattached_filter_destroy(ppp->pass_filter);
-			err = sk_unattached_filter_create(&ppp->pass_filter,
-							  &fprog);
+				ppp->pass_filter = NULL;
+			}
+			if (fprog.filter != NULL)
+				err = sk_unattached_filter_create(&ppp->pass_filter,
+								  &fprog);
+			else
+				err = 0;
 			kfree(code);
 			ppp_unlock(ppp);
 		}
@@ -784,10 +789,15 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 			};
 
 			ppp_lock(ppp);
-			if (ppp->active_filter)
+			if (ppp->active_filter) {
 				sk_unattached_filter_destroy(ppp->active_filter);
-			err = sk_unattached_filter_create(&ppp->active_filter,
-							  &fprog);
+				ppp->active_filter = NULL;
+			}
+			if (fprog.filter != NULL)
+				err = sk_unattached_filter_create(&ppp->active_filter,
+								  &fprog);
+			else
+				err = 0;
 			kfree(code);
 			ppp_unlock(ppp);
 		}

Re: [PATCH net 1/1] net: ppp: fix creating PPP pass and active filters

[Varka Bhadram]

On Sunday 13 July 2014 06:29 PM, Christoph Schulz wrote:
> From: Christoph Schulz <develop@kristov.de>
>
> Commit 568f194e8bd16c353ad50f9ab95d98b20578a39d ("net: ppp: use
> sk_unattached_filter api") inadvertently changed the logic when setting
> PPP pass and active filters. This applies to both the generic PPP subsystem
> implemented by drivers/net/ppp/ppp_generic.c and the ISDN PPP subsystem
> implemented by drivers/isdn/i4l/isdn_ppp.c. The original code in ppp_ioctl()
> (or isdn_ppp_ioctl(), resp.) handling PPPIOCSPASS and PPPIOCSACTIVE allowed to
> remove a pass/active filter previously set by using a filter of length zero.
> However, with the new code this is not possible anymore as this case is not
> explicitly checked for, which leads to passing NULL as a filter to
> sk_unattached_filter_create(). This results in returning EINVAL to the caller.
>
> Additionally, the variables ppp->pass_filter and ppp->active_filter (or
> is->pass_filter and is->active_filter, resp.) are not reset to NULL, although
> the filters they point to may have been destroyed by
> sk_unattached_filter_destroy(), so in this EINVAL case dangling pointers are
> left behind (provided the pointers were previously non-NULL).
>
> This patch corrects both problems by checking whether the filter passed is
> empty or non-empty, and prevents sk_unattached_filter_create() from being
> called in the first case. Moreover, the pointers are always reset to NULL
> as soon as sk_unattached_filter_destroy() returns.
>
> Signed-off-by: Christoph Schulz <develop@kristov.de>
> ---
> diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c
> index 61ac632..cd2f4c3 100644
> --- a/drivers/isdn/i4l/isdn_ppp.c
> +++ b/drivers/isdn/i4l/isdn_ppp.c
> @@ -644,9 +644,15 @@ isdn_ppp_ioctl(int min, struct file *file, unsigned int cmd, unsigned long arg)
>   		fprog.len = len;
>   		fprog.filter = code;
>   
> -		if (is->pass_filter)
> +		if (is->pass_filter) {
>   			sk_unattached_filter_destroy(is->pass_filter);
> -		err = sk_unattached_filter_create(&is->pass_filter, &fprog);
> +			is->pass_filter = NULL;
> +		}
> +		if (fprog.filter != NULL)
> +			err = sk_unattached_filter_create(&is->pass_filter,
> +							  &fprog);
> +		else
> +			err = 0;
>   		kfree(code);
>   
>   		return err;
> @@ -663,9 +669,15 @@ isdn_ppp_ioctl(int min, struct file *file, unsigned int cmd, unsigned long arg)
>   		fprog.len = len;
>   		fprog.filter = code;
>   
> -		if (is->active_filter)
> +		if (is->active_filter) {
>   			sk_unattached_filter_destroy(is->active_filter);
> -		err = sk_unattached_filter_create(&is->active_filter, &fprog);
> +			is->active_filter = NULL;
> +		}
> +		if (fprog.filter != NULL)
> +			err = sk_unattached_filter_create(&is->active_filter,
> +							  &fprog);
> +		else
> +			err = 0;
>   		kfree(code);
>   
>   		return err;
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 91d6c12..d0f6f93 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -763,10 +763,15 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>   			};
>   
>   			ppp_lock(ppp);
> -			if (ppp->pass_filter)
> +			if (ppp->pass_filter) {
>   				sk_unattached_filter_destroy(ppp->pass_filter);
> -			err = sk_unattached_filter_create(&ppp->pass_filter,
> -							  &fprog);
> +				ppp->pass_filter = NULL;
> +			}
> +			if (fprog.filter != NULL)
> +				err = sk_unattached_filter_create(&ppp->pass_filter,
> +								  &fprog);
> +			else
> +				err = 0;
>   			kfree(code);
>   			ppp_unlock(ppp);
>   		}
> @@ -784,10 +789,15 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>   			};
>   
>   			ppp_lock(ppp);
> -			if (ppp->active_filter)
> +			if (ppp->active_filter) {
>   				sk_unattached_filter_destroy(ppp->active_filter);
> -			err = sk_unattached_filter_create(&ppp->active_filter,
> -							  &fprog);
> +				ppp->active_filter = NULL;
> +			}
> +			if (fprog.filter != NULL)
> +				err = sk_unattached_filter_create(&ppp->active_filter,
> +								  &fprog);
> +			else
> +				err = 0;
>   			kfree(code);
>   			ppp_unlock(ppp);
>   		}
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

checkpatch warnings on this patch

-- 
Regards,
Varka Bhadram

Re: [PATCH net 1/1] net: ppp: fix creating PPP pass and active filters

[Christoph Schulz]

Hello!

Varka Bhadram schrieb am Sun, 13 Jul 2014 20:24:34 +0530:

> checkpatch warnings on this patch

Yes, I know. But could you please give me a hint how to indent this  
properly such that it doesn't conflict with any formatting rules I  
possibly don't even know about?

+                        err = sk_unattached_filter_create(&is->pass_filter,
+                                                          &fprog);

Can I use

+                        err = sk_unattached_filter_create(
+                                &is->pass_filter, &fprog);

or similar? I do not want to rename variables to fit the line into 80  
characteres...


Regards,

Christoph Schulz

Re: [PATCH net 1/1] net: ppp: fix creating PPP pass and active filters

[Daniel Borkmann]

On 07/13/2014 06:07 PM, Christoph Schulz wrote:
> Hello!
>
> Varka Bhadram schrieb am Sun, 13 Jul 2014 20:24:34 +0530:
>
>> checkpatch warnings on this patch
>
> Yes, I know. But could you please give me a hint how to indent this properly such that it doesn't conflict with any formatting rules I possibly don't even know about?
>
> +                        err = sk_unattached_filter_create(&is->pass_filter,
> +                                                          &fprog);

I think going with the first variant is just fine.

> Can I use
>
> +                        err = sk_unattached_filter_create(
> +                                &is->pass_filter, &fprog);
>
> or similar? I do not want to rename variables to fit the line into 80 characteres...

Re: [PATCH net 1/1] net: ppp: fix creating PPP pass and active filters

[Christoph Schulz]

Hello!

Daniel Borkmann schrieb am Sun, 13 Jul 2014 20:51:45 +0200:

> I think going with the first variant is just fine.

Well, then I need not change anything, do I? This first variant causes  
checkpatch to warn twice about a line exceeding 80 characters:

WARNING: line over 80 characters
#83: FILE: drivers/net/ppp/ppp_generic.c:771:
+                               err =  
sk_unattached_filter_create(&ppp->pass_filter,

WARNING: line over 80 characters
#102: FILE: drivers/net/ppp/ppp_generic.c:797:
+                               err =  
sk_unattached_filter_create(&ppp->active_filter,

But I don't see how to shorten them. This is because  
"sk_unattached_filter_create" is such a long identifier...

Any proposals what I can do in order to get the fix accepted?


Thank you in advance,

Christoph Schulz

Re: [PATCH net 1/1] net: ppp: fix creating PPP pass and active filters

[Varka Bhadram]

On 07/15/2014 10:24 AM, Christoph Schulz wrote:
> Hello!
>
> Daniel Borkmann schrieb am Sun, 13 Jul 2014 20:51:45 +0200:
>
>> I think going with the first variant is just fine.
>
> Well, then I need not change anything, do I? This first variant causes 
> checkpatch to warn twice about a line exceeding 80 characters:
>
> WARNING: line over 80 characters
> #83: FILE: drivers/net/ppp/ppp_generic.c:771:
> +                               err = 
> sk_unattached_filter_create(&ppp->pass_filter,
>
> WARNING: line over 80 characters
> #102: FILE: drivers/net/ppp/ppp_generic.c:797:
> +                               err = 
> sk_unattached_filter_create(&ppp->active_filter,
>
> But I don't see how to shorten them. This is because 
> "sk_unattached_filter_create" is such a long identifier...
>
> Any proposals what I can do in order to get the fix accepted?
>
>
There are can be two solutions:

1. Replace the long function name (sk_unattached_filter_create) with the simple macro.
    We can define the macro locally with our driver name.I don't know how many people
    will accept it. For this we have to change the entire filter framework.

2. Shorten the argument name 'pass_filter/active_filter'.
    For this also we need to change the entire framework. People many not be accept it.

-- 
Regards,
Varka Bhadram.

Re: [PATCH net 1/1] net: ppp: fix creating PPP pass and active filters

[Christoph Schulz]

Hello!

Am 15.07.2014 07:18, schrieb Varka Bhadram:
> There are can be two solutions:
> 
> 1. Replace the long function name (sk_unattached_filter_create) with the simple macro.
>     We can define the macro locally with our driver name.I don't know how many people
>     will accept it. For this we have to change the entire filter framework.
> 
> 2. Shorten the argument name 'pass_filter/active_filter'.
>     For this also we need to change the entire framework. People many not be accept it.

Is the 80 characters limit really that strong?
Documentation/SubmittingPatches contains the paragraph:

> The style checker should be viewed as
> a guide not as the final word.  If your code looks better with
> a violation then its probably best left alone.

IMHO defining and using local macros only to shorten two lines does not
improve code readability. Note that we talk about 83 and 85 characters,
respectively, not about >= 100.


Regards,

Christoph Schulz

Re: [PATCH net 1/1] net: ppp: fix creating PPP pass and active filters

[Daniel Borkmann]

On 07/15/2014 06:54 AM, Christoph Schulz wrote:
> Hello!
>
> Daniel Borkmann schrieb am Sun, 13 Jul 2014 20:51:45 +0200:
>
>> I think going with the first variant is just fine.
>
> Well, then I need not change anything, do I? This first variant causes checkpatch to warn twice about a line exceeding 80 characters:
>
> WARNING: line over 80 characters
> #83: FILE: drivers/net/ppp/ppp_generic.c:771:
> +                               err = sk_unattached_filter_create(&ppp->pass_filter,
>
> WARNING: line over 80 characters
> #102: FILE: drivers/net/ppp/ppp_generic.c:797:
> +                               err = sk_unattached_filter_create(&ppp->active_filter,
>
> But I don't see how to shorten them. This is because "sk_unattached_filter_create" is such a long identifier...

Well, this is a soft-limit, and in this particular circumstance
it should be fine to use your original proposal. I don't see how
Varka's suggestions make this better in _any way_, rather the
very opposite.

Re: [PATCH net 1/1] net: ppp: fix creating PPP pass and active filters

[Alexei Starovoitov]

On Mon, Jul 14, 2014 at 11:59 PM, Daniel Borkmann <dborkman@redhat.com> wrote:
> On 07/15/2014 06:54 AM, Christoph Schulz wrote:
>>
>> Hello!
>>
>> Daniel Borkmann schrieb am Sun, 13 Jul 2014 20:51:45 +0200:
>>
>>> I think going with the first variant is just fine.
>>
>>
>> Well, then I need not change anything, do I? This first variant causes
>> checkpatch to warn twice about a line exceeding 80 characters:
>>
>> WARNING: line over 80 characters
>> #83: FILE: drivers/net/ppp/ppp_generic.c:771:
>> +                               err =
>> sk_unattached_filter_create(&ppp->pass_filter,
>>
>> WARNING: line over 80 characters
>> #102: FILE: drivers/net/ppp/ppp_generic.c:797:
>> +                               err =
>> sk_unattached_filter_create(&ppp->active_filter,
>>
>> But I don't see how to shorten them. This is because
>> "sk_unattached_filter_create" is such a long identifier...
>
>
> Well, this is a soft-limit, and in this particular circumstance
> it should be fine to use your original proposal. I don't see how
> Varka's suggestions make this better in _any way_, rather the
> very opposite.

+1

Re: [PATCH net 1/1] net: ppp: fix creating PPP pass and active filters

[Christoph Schulz]

Hello!

Alexei Starovoitov schrieb am Tue, 15 Jul 2014 08:38:58 -0700:

>> Well, this is a soft-limit, and in this particular circumstance
>> it should be fine to use your original proposal. I don't see how
>> Varka's suggestions make this better in _any way_, rather the
>> very opposite.
>
> +1

Fine. So I need not change anything, right? How can I change (or  
trigger changing) the patch's state from "Changes Requested" to "Under  
Review" again without resubmitting it without any modifications? Or  
will that happen eventually anyway after some time has elapsed?


Best regards,

Christoph Schulz

Re: [PATCH net 1/1] net: ppp: fix creating PPP pass and active filters

[David Miller]

From: Christoph Schulz <develop@kristov.de>
Date: Wed, 16 Jul 2014 20:47:55 +0200

> Hello!
> 
> Alexei Starovoitov schrieb am Tue, 15 Jul 2014 08:38:58 -0700:
> 
>>> Well, this is a soft-limit, and in this particular circumstance
>>> it should be fine to use your original proposal. I don't see how
>>> Varka's suggestions make this better in _any way_, rather the
>>> very opposite.
>>
>> +1
> 
> Fine. So I need not change anything, right? How can I change (or
> trigger changing) the patch's state from "Changes Requested" to "Under
> Review" again without resubmitting it without any modifications? Or
> will that happen eventually anyway after some time has elapsed?

Please just submit the patch again.