| Message ID | 53BF8DEC.4000307@kristov.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Jul 11, 2014 at 9:10 AM, Christoph Schulz <develop@kristov.de> wrote: > From: Christoph Schulz <develop@kristov.de> > > Commit 568f194e8bd16c353ad50f9ab95d98b20578a39d ("net: ppp: use > sk_unattached_filter api") causes sk_chk_filter() to be called twice when > setting a pass or active filter. The first call is from within get_filter(). > The second one is through the call chain > > ppp_ioctl() --> sk_unattached_filter_create() > --> __sk_prepare_filter() > --> sk_chk_filter() > > However, sk_chk_filter() is not idempotent as it sometimes replaces filter > codes. So running it a second time over the same filter does not work and It's a good thing not to call sk_chk_filter() twice, but the commit log is incorrect. sk_chk_filter() doesn't replace filter codes anymore. > yields EINVAL. The net effect is that setting pass and/or active PPP filters > does not work anymore, since sk_unattached_filter_create() always returns > EINVAL due to the second (and erroneous) call to sk_chk_filter(), regardless > whether the filter was sane or not. So this patch simply removes the call to > sk_chk_filter() from within get_filter(). This is safe as the filter will > be checked by sk_chk_filter() later anyway. > > This error is found in exactly the same way in the isdn4linux PPP driver, so > it is fixed there the same way. > > 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..a333b7f 100644 > --- a/drivers/isdn/i4l/isdn_ppp.c > +++ b/drivers/isdn/i4l/isdn_ppp.c > @@ -442,7 +442,7 @@ static int get_filter(void __user *arg, struct sock_filter **p) > { > struct sock_fprog uprog; > struct sock_filter *code = NULL; > - int len, err; > + int len; > > if (copy_from_user(&uprog, arg, sizeof(uprog))) > return -EFAULT; > @@ -458,12 +458,6 @@ static int get_filter(void __user *arg, struct sock_filter **p) > if (IS_ERR(code)) > return PTR_ERR(code); > > - err = sk_chk_filter(code, uprog.len); > - if (err) { > - kfree(code); > - return err; > - } > - > *p = code; > return uprog.len; > } > diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c > index 91d6c12..e2f20f8 100644 > --- a/drivers/net/ppp/ppp_generic.c > +++ b/drivers/net/ppp/ppp_generic.c > @@ -539,7 +539,7 @@ static int get_filter(void __user *arg, struct sock_filter **p) > { > struct sock_fprog uprog; > struct sock_filter *code = NULL; > - int len, err; > + int len; > > if (copy_from_user(&uprog, arg, sizeof(uprog))) > return -EFAULT; > @@ -554,12 +554,6 @@ static int get_filter(void __user *arg, struct sock_filter **p) > if (IS_ERR(code)) > return PTR_ERR(code); > > - err = sk_chk_filter(code, uprog.len); > - if (err) { > - kfree(code); > - return err; > - } > - > *p = code; > return uprog.len; > } > -- > 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 -- 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/12/2014 05:59 AM, Alexei Starovoitov wrote: > On Fri, Jul 11, 2014 at 9:10 AM, Christoph Schulz <develop@kristov.de> wrote: >> From: Christoph Schulz <develop@kristov.de> >> >> Commit 568f194e8bd16c353ad50f9ab95d98b20578a39d ("net: ppp: use >> sk_unattached_filter api") causes sk_chk_filter() to be called twice when >> setting a pass or active filter. The first call is from within get_filter(). >> The second one is through the call chain >> >> ppp_ioctl() --> sk_unattached_filter_create() >> --> __sk_prepare_filter() >> --> sk_chk_filter() >> >> However, sk_chk_filter() is not idempotent as it sometimes replaces filter >> codes. So running it a second time over the same filter does not work and > > It's a good thing not to call sk_chk_filter() twice, but the commit > log is incorrect. > sk_chk_filter() doesn't replace filter codes anymore. Exactly. -- 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 Sat, 12 Jul 2014 05:59:46 +0200: >> However, sk_chk_filter() is not idempotent as it sometimes replaces filter >> codes. So running it a second time over the same filter does not work and > > It's a good thing not to call sk_chk_filter() twice, but the commit > log is incorrect. > sk_chk_filter() doesn't replace filter codes anymore. Fair enough. Then how should I correctly proceed to submit this patch which fixes a bug in the 3.15 branch (only)? In 3.15.x filter codes _are_ replaced (I just checked the code in 3.15.5). And I originally based my analysis on 3.15.1. Your statement makes the patch an optional improvement for 3.16.x, but it's a necessary fix for 3.15.x. Do I need to submit this patch two times with different commit logs? 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 Sat, Jul 12, 2014 at 11:11 PM, Christoph Schulz <develop@kristov.de> wrote: > Hello! > > Alexei Starovoitov schrieb am Sat, 12 Jul 2014 05:59:46 +0200: > > >>> However, sk_chk_filter() is not idempotent as it sometimes replaces >>> filter >>> codes. So running it a second time over the same filter does not work and >> >> >> It's a good thing not to call sk_chk_filter() twice, but the commit >> log is incorrect. >> sk_chk_filter() doesn't replace filter codes anymore. > > > Fair enough. Then how should I correctly proceed to submit this patch which > fixes a bug in the 3.15 branch (only)? In 3.15.x filter codes _are_ replaced > (I just checked the code in 3.15.5). And I originally based my analysis on > 3.15.1. Your statement makes the patch an optional improvement for 3.16.x, > but it's a necessary fix for 3.15.x. Do I need to submit this patch two > times with different commit logs? I think this patch still makes sense for 'net-next' as cleanup. Just explain it correctly in the log. It's not needed for 'net'. As far as stable for 3.15, I'm not yet sure what exactly the problem you're hitting. The way you describe it, the whole ppp filtering shouldn't be working in 3.15... Also it sounds like you've created a patch out of 3.15 tree, but marked it as 'net-next'. That's not the right. If the tag is 'net-next' it obviously should be based on net-next tree. -- 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 Sun, 13 Jul 2014 03:44:59 +0200: > I think this patch still makes sense for 'net-next' as cleanup. Just > explain it > correctly in the log. It's not needed for 'net'. OK. > As far as stable for 3.15, I'm not yet sure what exactly the problem > you're hitting. The way you describe it, the whole ppp filtering shouldn't > be working in 3.15... I'm afraid this is true. The pppd daemon fails to set the filter(s) with the message "Couldn't set pass-filter in kernel: Invalid argument". (It doesn't try to set the active-filter because it gives up after the first error.) For 3.15.x, you have to apply both of my patches to make it work. For 'net', presumably only the second patch ("fix creating PPP pass and active filters") is necessary because as you wrote, sk_chk_filter() can be called multiple times there. See also my comment in our bug tracker where the creation of the filter is compared between 3.14.x and 3.15.x (I'm afraid the description is in German): https://ssl.nettworks.org/bugs/browse/FFL-858?focusedCommentId=17289&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17289 > Also it sounds like you've created a patch out of 3.15 tree, but marked it > as 'net-next'. That's not the right. If the tag is 'net-next' it obviously > should be based on net-next tree. Yes, this was my fault. I'm sorry. I will resubmit the patches soon. 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/12/2014 11:11 PM, Christoph Schulz wrote: > Hello! > > Alexei Starovoitov schrieb am Sat, 12 Jul 2014 05:59:46 +0200: > >>> However, sk_chk_filter() is not idempotent as it sometimes replaces filter >>> codes. So running it a second time over the same filter does not work and >> >> It's a good thing not to call sk_chk_filter() twice, but the commit >> log is incorrect. >> sk_chk_filter() doesn't replace filter codes anymore. > > Fair enough. Then how should I correctly proceed to submit this patch which > fixes a bug in the 3.15 branch (only)? In 3.15.x filter codes _are_ replaced > (I just checked the code in 3.15.5). And I originally based my analysis on > 3.15.1. Your statement makes the patch an optional improvement for 3.16.x, > but it's a necessary fix for 3.15.x. Do I need to submit this patch two times > with different commit logs? I think the patch makes sense, and you could submit it against net tree (so 'PATCH net' in subject) with a slightly different commit log at the beginning, but the rest could stay explaining that that's the case for 3.15. By that, this could then be picked up into the net tree and thus Dave can queue it for stable inclusion. If you need any help, let me know. Thanks again, Daniel -- 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! Am 13.07.2014 12:27, schrieb Daniel Borkmann: > I think the patch makes sense, and you could submit it against net tree > (so 'PATCH net' in subject) with a slightly different commit log at the > beginning, but the rest could stay explaining that that's the case for > 3.15. By that, this could then be picked up into the net tree and thus > Dave can queue it for stable inclusion. If you need any help, let me know. I have just resubmitted the patch and hope I did it better this time. Please let me know whether the commit log is correct now. Thank you, 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
diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c index 61ac632..a333b7f 100644 --- a/drivers/isdn/i4l/isdn_ppp.c +++ b/drivers/isdn/i4l/isdn_ppp.c @@ -442,7 +442,7 @@ static int get_filter(void __user *arg, struct sock_filter **p) { struct sock_fprog uprog; struct sock_filter *code = NULL; - int len, err; + int len; if (copy_from_user(&uprog, arg, sizeof(uprog))) return -EFAULT; @@ -458,12 +458,6 @@ static int get_filter(void __user *arg, struct sock_filter **p) if (IS_ERR(code)) return PTR_ERR(code); - err = sk_chk_filter(code, uprog.len); - if (err) { - kfree(code); - return err; - } - *p = code; return uprog.len; } diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c index 91d6c12..e2f20f8 100644 --- a/drivers/net/ppp/ppp_generic.c +++ b/drivers/net/ppp/ppp_generic.c @@ -539,7 +539,7 @@ static int get_filter(void __user *arg, struct sock_filter **p) { struct sock_fprog uprog; struct sock_filter *code = NULL; - int len, err; + int len; if (copy_from_user(&uprog, arg, sizeof(uprog))) return -EFAULT; @@ -554,12 +554,6 @@ static int get_filter(void __user *arg, struct sock_filter **p) if (IS_ERR(code)) return PTR_ERR(code); - err = sk_chk_filter(code, uprog.len); - if (err) { - kfree(code); - return err; - } - *p = code; return uprog.len; }