| Message ID | 53C28297.4080507@kristov.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
--
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
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... -- 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
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
--
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
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.
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 -- 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
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. -- 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
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 -- 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
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 -- 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
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. -- 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
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); }