Patchwork [1/9] net classifier: dont allow filters on semi-classful qdisc

login
register
mail settings
Submitter stephen hemminger
Date Aug. 6, 2010, 7:35 p.m.
Message ID <20100806193558.580890552@vyatta.com>
Download mbox | patch
Permalink /patch/61140/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

stephen hemminger - Aug. 6, 2010, 7:35 p.m.
There are several qdisc which only support a single class (sfq, mq, tbf)
and the kernel would dereference a null pointer (bind_tcf), if a user
attempted to apply a filter one of these classes.

This patch changes the tcf_bind_filter to return an error in
these cases.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


---
This needs to go in net-2.6 and stable.

 include/net/pkt_cls.h   |   12 +++++++++---
 net/sched/cls_basic.c   |    4 +++-
 net/sched/cls_fw.c      |    6 ++++--
 net/sched/cls_route.c   |    4 +++-
 net/sched/cls_tcindex.c |    4 +++-
 net/sched/cls_u32.c     |    4 +++-
 6 files changed, 25 insertions(+), 9 deletions(-)



--
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
Jarek Poplawski - Aug. 6, 2010, 9:24 p.m.
Stephen Hemminger wrote, On -10.01.-28163 20:59:

> There are several qdisc which only support a single class (sfq, mq, tbf)
> and the kernel would dereference a null pointer (bind_tcf), if a user
> attempted to apply a filter one of these classes.


mq and tbf can't have this issue because they don't have
.tcf_chain class method. sfq should support it on purpose
after this patch:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7d2681a6ff4f9ab5e48d02550b4c6338f1638998
and needs tiny fix only.


Jarek P.

> 
> This patch changes the tcf_bind_filter to return an error in
> these cases.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> 
> ---
> This needs to go in net-2.6 and stable.
> 
>  include/net/pkt_cls.h   |   12 +++++++++---
>  net/sched/cls_basic.c   |    4 +++-
>  net/sched/cls_fw.c      |    6 ++++--
>  net/sched/cls_route.c   |    4 +++-
>  net/sched/cls_tcindex.c |    4 +++-
>  net/sched/cls_u32.c     |    4 +++-
>  6 files changed, 25 insertions(+), 9 deletions(-)
> 
> --- a/include/net/pkt_cls.h	2010-08-06 11:51:18.903581556 -0700
> +++ b/include/net/pkt_cls.h	2010-08-06 12:20:02.072241508 -0700
> @@ -40,15 +40,21 @@ cls_set_class(struct tcf_proto *tp, unsi
>  	return old_cl;
>  }
>  
> -static inline void
> +static inline int
>  tcf_bind_filter(struct tcf_proto *tp, struct tcf_result *r, unsigned long base)
>  {
> +	const struct Qdisc_class_ops *cops = tp->q->ops->cl_ops;
>  	unsigned long cl;
>  
> -	cl = tp->q->ops->cl_ops->bind_tcf(tp->q, base, r->classid);
> +	if (!cops->bind_tcf)
> +		return -EINVAL;
> +
> +	cl = cops->bind_tcf(tp->q, base, r->classid);
>  	cl = cls_set_class(tp, &r->class, cl);
>  	if (cl)
> -		tp->q->ops->cl_ops->unbind_tcf(tp->q, cl);
> +		cops->unbind_tcf(tp->q, cl);
> +
> +	return 0;
>  }
>  
>  static inline void
> --- a/net/sched/cls_basic.c	2010-08-06 11:51:18.923582342 -0700
> +++ b/net/sched/cls_basic.c	2010-08-06 11:55:13.292553190 -0700
> @@ -153,7 +153,9 @@ static inline int basic_set_parms(struct
>  
>  	if (tb[TCA_BASIC_CLASSID]) {
>  		f->res.classid = nla_get_u32(tb[TCA_BASIC_CLASSID]);
> -		tcf_bind_filter(tp, &f->res, base);
> +		err = tcf_bind_filter(tp, &f->res, base);
> +		if (err)
> +			goto errout;
>  	}
>  
>  	tcf_exts_change(tp, &f->exts, &e);
> --- a/net/sched/cls_fw.c	2010-08-06 11:51:18.943583126 -0700
> +++ b/net/sched/cls_fw.c	2010-08-06 11:55:39.085476144 -0700
> @@ -206,10 +206,11 @@ fw_change_attrs(struct tcf_proto *tp, st
>  	if (err < 0)
>  		return err;
>  
> -	err = -EINVAL;
>  	if (tb[TCA_FW_CLASSID]) {
>  		f->res.classid = nla_get_u32(tb[TCA_FW_CLASSID]);
> -		tcf_bind_filter(tp, &f->res, base);
> +		err = tcf_bind_filter(tp, &f->res, base);
> +		if (err)
> +			goto errout;
>  	}
>  
>  #ifdef CONFIG_NET_CLS_IND
> @@ -220,6 +221,7 @@ fw_change_attrs(struct tcf_proto *tp, st
>  	}
>  #endif /* CONFIG_NET_CLS_IND */
>  
> +	err = -EINVAL;
>  	if (tb[TCA_FW_MASK]) {
>  		mask = nla_get_u32(tb[TCA_FW_MASK]);
>  		if (mask != head->mask)
> --- a/net/sched/cls_route.c	2010-08-06 11:51:18.959583757 -0700
> +++ b/net/sched/cls_route.c	2010-08-06 11:55:50.077870498 -0700
> @@ -412,7 +412,9 @@ static int route4_set_parms(struct tcf_p
>  
>  	if (tb[TCA_ROUTE4_CLASSID]) {
>  		f->res.classid = nla_get_u32(tb[TCA_ROUTE4_CLASSID]);
> -		tcf_bind_filter(tp, &f->res, base);
> +		err = tcf_bind_filter(tp, &f->res, base);
> +		if (err)
> +			goto errout;
>  	}
>  
>  	tcf_exts_change(tp, &f->exts, &e);
> --- a/net/sched/cls_tcindex.c	2010-08-06 11:51:18.999585326 -0700
> +++ b/net/sched/cls_tcindex.c	2010-08-06 11:56:01.486283847 -0700
> @@ -295,7 +295,9 @@ tcindex_set_parms(struct tcf_proto *tp, 
>  
>  	if (tb[TCA_TCINDEX_CLASSID]) {
>  		cr.res.classid = nla_get_u32(tb[TCA_TCINDEX_CLASSID]);
> -		tcf_bind_filter(tp, &cr.res, base);
> +		err = tcf_bind_filter(tp, &cr.res, base);
> +		if (err)
> +			goto errout;
>  	}
>  
>  	tcf_exts_change(tp, &cr.exts, &e);
> --- a/net/sched/cls_u32.c	2010-08-06 11:51:19.019586112 -0700
> +++ b/net/sched/cls_u32.c	2010-08-06 11:56:12.390678703 -0700
> @@ -528,7 +528,9 @@ static int u32_set_parms(struct tcf_prot
>  	}
>  	if (tb[TCA_U32_CLASSID]) {
>  		n->res.classid = nla_get_u32(tb[TCA_U32_CLASSID]);
> -		tcf_bind_filter(tp, &n->res, base);
> +		err = tcf_bind_filter(tp, &n->res, base);
> +		if (err)
> +			goto errout;
>  	}
>  
>  #ifdef CONFIG_NET_CLS_IND
> 
> 
> --
> 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
stephen hemminger - Aug. 6, 2010, 9:58 p.m.
On Fri, 06 Aug 2010 23:24:47 +0200
Jarek Poplawski <jarkao2@gmail.com> wrote:

> Stephen Hemminger wrote, On -10.01.-28163 20:59:
> 
> > There are several qdisc which only support a single class (sfq, mq, tbf)
> > and the kernel would dereference a null pointer (bind_tcf), if a user
> > attempted to apply a filter one of these classes.
> 
> 
> mq and tbf can't have this issue because they don't have
> .tcf_chain class method. sfq should support it on purpose
> after this patch:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7d2681a6ff4f9ab5e48d02550b4c6338f1638998
> and needs tiny fix only.

Probably best to fix both ways.  Fix sfq to allow filters to
be chained, and fix API to prevent refuse to allow qdisc to
register with tcf_chain && !bind_tcf
--
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
Jarek Poplawski - Aug. 6, 2010, 10:26 p.m.
Stephen Hemminger wrote, On 06.08.2010 23:58:

> On Fri, 06 Aug 2010 23:24:47 +0200
> Jarek Poplawski <jarkao2@gmail.com> wrote:
> 
>> Stephen Hemminger wrote, On -10.01.-28163 20:59:
>>
>>> There are several qdisc which only support a single class (sfq, mq, tbf)
>>> and the kernel would dereference a null pointer (bind_tcf), if a user
>>> attempted to apply a filter one of these classes.
>>
>>
>> mq and tbf can't have this issue because they don't have
>> .tcf_chain class method. sfq should support it on purpose
>> after this patch:
>>
>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7d2681a6ff4f9ab5e48d02550b4c6338f1638998
>> and needs tiny fix only.
> 
> Probably best to fix both ways.  Fix sfq to allow filters to
> be chained, and fix API to prevent refuse to allow qdisc to
> register with tcf_chain && !bind_tcf
> 

Yes, but your patch needs a different changelog and I'm not sure
it's necessary for stable (there should be no other such cases
for now).

Jarek P.
--
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
David Miller - Aug. 8, 2010, 5:59 a.m.
From: Jarek Poplawski <jarkao2@gmail.com>
Date: Sat, 07 Aug 2010 00:26:17 +0200

> Stephen Hemminger wrote, On 06.08.2010 23:58:
> 
>> On Fri, 06 Aug 2010 23:24:47 +0200
>> Jarek Poplawski <jarkao2@gmail.com> wrote:
>> 
>>> Stephen Hemminger wrote, On -10.01.-28163 20:59:
>>>
>>>> There are several qdisc which only support a single class (sfq, mq, tbf)
>>>> and the kernel would dereference a null pointer (bind_tcf), if a user
>>>> attempted to apply a filter one of these classes.
>>>
>>>
>>> mq and tbf can't have this issue because they don't have
>>> .tcf_chain class method. sfq should support it on purpose
>>> after this patch:
>>>
>>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7d2681a6ff4f9ab5e48d02550b4c6338f1638998
>>> and needs tiny fix only.
>> 
>> Probably best to fix both ways.  Fix sfq to allow filters to
>> be chained, and fix API to prevent refuse to allow qdisc to
>> register with tcf_chain && !bind_tcf
>> 
> 
> Yes, but your patch needs a different changelog and I'm not sure
> it's necessary for stable (there should be no other such cases
> for now).

Right.

I'm tossing this series since there are dependencies upon this first
patch for which we've arrived at a different solution (Jarek's
patch).
--
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

Patch

--- a/include/net/pkt_cls.h	2010-08-06 11:51:18.903581556 -0700
+++ b/include/net/pkt_cls.h	2010-08-06 12:20:02.072241508 -0700
@@ -40,15 +40,21 @@  cls_set_class(struct tcf_proto *tp, unsi
 	return old_cl;
 }
 
-static inline void
+static inline int
 tcf_bind_filter(struct tcf_proto *tp, struct tcf_result *r, unsigned long base)
 {
+	const struct Qdisc_class_ops *cops = tp->q->ops->cl_ops;
 	unsigned long cl;
 
-	cl = tp->q->ops->cl_ops->bind_tcf(tp->q, base, r->classid);
+	if (!cops->bind_tcf)
+		return -EINVAL;
+
+	cl = cops->bind_tcf(tp->q, base, r->classid);
 	cl = cls_set_class(tp, &r->class, cl);
 	if (cl)
-		tp->q->ops->cl_ops->unbind_tcf(tp->q, cl);
+		cops->unbind_tcf(tp->q, cl);
+
+	return 0;
 }
 
 static inline void
--- a/net/sched/cls_basic.c	2010-08-06 11:51:18.923582342 -0700
+++ b/net/sched/cls_basic.c	2010-08-06 11:55:13.292553190 -0700
@@ -153,7 +153,9 @@  static inline int basic_set_parms(struct
 
 	if (tb[TCA_BASIC_CLASSID]) {
 		f->res.classid = nla_get_u32(tb[TCA_BASIC_CLASSID]);
-		tcf_bind_filter(tp, &f->res, base);
+		err = tcf_bind_filter(tp, &f->res, base);
+		if (err)
+			goto errout;
 	}
 
 	tcf_exts_change(tp, &f->exts, &e);
--- a/net/sched/cls_fw.c	2010-08-06 11:51:18.943583126 -0700
+++ b/net/sched/cls_fw.c	2010-08-06 11:55:39.085476144 -0700
@@ -206,10 +206,11 @@  fw_change_attrs(struct tcf_proto *tp, st
 	if (err < 0)
 		return err;
 
-	err = -EINVAL;
 	if (tb[TCA_FW_CLASSID]) {
 		f->res.classid = nla_get_u32(tb[TCA_FW_CLASSID]);
-		tcf_bind_filter(tp, &f->res, base);
+		err = tcf_bind_filter(tp, &f->res, base);
+		if (err)
+			goto errout;
 	}
 
 #ifdef CONFIG_NET_CLS_IND
@@ -220,6 +221,7 @@  fw_change_attrs(struct tcf_proto *tp, st
 	}
 #endif /* CONFIG_NET_CLS_IND */
 
+	err = -EINVAL;
 	if (tb[TCA_FW_MASK]) {
 		mask = nla_get_u32(tb[TCA_FW_MASK]);
 		if (mask != head->mask)
--- a/net/sched/cls_route.c	2010-08-06 11:51:18.959583757 -0700
+++ b/net/sched/cls_route.c	2010-08-06 11:55:50.077870498 -0700
@@ -412,7 +412,9 @@  static int route4_set_parms(struct tcf_p
 
 	if (tb[TCA_ROUTE4_CLASSID]) {
 		f->res.classid = nla_get_u32(tb[TCA_ROUTE4_CLASSID]);
-		tcf_bind_filter(tp, &f->res, base);
+		err = tcf_bind_filter(tp, &f->res, base);
+		if (err)
+			goto errout;
 	}
 
 	tcf_exts_change(tp, &f->exts, &e);
--- a/net/sched/cls_tcindex.c	2010-08-06 11:51:18.999585326 -0700
+++ b/net/sched/cls_tcindex.c	2010-08-06 11:56:01.486283847 -0700
@@ -295,7 +295,9 @@  tcindex_set_parms(struct tcf_proto *tp, 
 
 	if (tb[TCA_TCINDEX_CLASSID]) {
 		cr.res.classid = nla_get_u32(tb[TCA_TCINDEX_CLASSID]);
-		tcf_bind_filter(tp, &cr.res, base);
+		err = tcf_bind_filter(tp, &cr.res, base);
+		if (err)
+			goto errout;
 	}
 
 	tcf_exts_change(tp, &cr.exts, &e);
--- a/net/sched/cls_u32.c	2010-08-06 11:51:19.019586112 -0700
+++ b/net/sched/cls_u32.c	2010-08-06 11:56:12.390678703 -0700
@@ -528,7 +528,9 @@  static int u32_set_parms(struct tcf_prot
 	}
 	if (tb[TCA_U32_CLASSID]) {
 		n->res.classid = nla_get_u32(tb[TCA_U32_CLASSID]);
-		tcf_bind_filter(tp, &n->res, base);
+		err = tcf_bind_filter(tp, &n->res, base);
+		if (err)
+			goto errout;
 	}
 
 #ifdef CONFIG_NET_CLS_IND