diff mbox

[BUG] net_cls: Panic occured when net_cls subsystem use

Message ID 20090601204957.GA2760@ami.dom.local
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Jarek Poplawski June 1, 2009, 8:49 p.m. UTC 
On Mon, Jun 01, 2009 at 09:03:30AM -0400, jamal wrote:
...
> How about going back to your original idea of defining tp_created? With
> apologies to Minoru (he must be thinking we are lunatics by now), how
> does the attached changed patch look to you?
> 
> Before you throw another rock,

Actually, I'd insist with the old rock and handling that other rude
u32 case, at least until it's fixed in place. So I attach my version
of your patch (additionally I removed a pair of braces because of
checkpatch warning).

Alas, I still think we don't need to change so much in -stable to
fix the cls_cgroup oops, so I attach a patch which I think is
enough for -stable and probably -net too. It could be "reverted"
in -net-next just after applying cls_api patch. Of course, treat
it only as my humble proposal, and feel free to recommend to David
your version, no problem (really).

> there is another issue which will be
> caused by this rude misconfig:
> "replace" really means "get rid of the old and add this new one".
> But for the last 50 years we do not "get rid of the old". I cant think
> of a clean way to do it sans shaving one of the kittens. One simple
> thing to do is to printk a warning when detecting this error. I think
> one needs to draw a line where bad config affects your life - in this
> case i dont think it is worth big changes..

Of course I'm against any printk on shaving the kitten...

Cheers,
Jarek P.
-----------------------> patch #1

 net/sched/cls_api.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 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

Comments

jamal June 1, 2009, 10:06 p.m. UTC | #1 
On Mon, 2009-06-01 at 22:49 +0200, Jarek Poplawski wrote:

> 
> Actually, I'd insist with the old rock and handling that other rude
> u32 case, at least until it's fixed in place. So I attach my version
> of your patch (additionally I removed a pair of braces because of
> checkpatch warning).

Sure, it doesnt complicate anything - Minoru, this is the version to go
for.

> Alas, I still think we don't need to change so much in -stable to
> fix the cls_cgroup oops, so I attach a patch which I think is
> enough for -stable and probably -net too. It could be "reverted"
> in -net-next just after applying cls_api patch. Of course, treat
> it only as my humble proposal, and feel free to recommend to David
> your version, no problem (really).


My view is the same - that the second patch doesnt fix the root
cause; and it is not that complicated to fix the root cause. So I humbly
disagree with you.

The issue is that a classifer (cls_group in this example) is being
misconfigured. It rejects the config - but the tp has already been
added. 
It then tries to use the tp in the fast and fails. 

If you look as closely as you did with the patch i posted, youd find
ways to construct similar hostile misconfigs for other classifiers.
You just need to create the scenario where the attributes will fail to
validate. 

I actually suspect the most common scenario for such a failure is not
that head is null (I doubt in Minoru case that allocation will fail);
rather it is some reference to head->something.

cheers,
jamal

--
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 June 2, 2009, 6:26 a.m. UTC | #2 
On Mon, Jun 01, 2009 at 06:06:03PM -0400, jamal wrote:
> On Mon, 2009-06-01 at 22:49 +0200, Jarek Poplawski wrote:
> 
> > 
> > Actually, I'd insist with the old rock and handling that other rude
> > u32 case, at least until it's fixed in place. So I attach my version
> > of your patch (additionally I removed a pair of braces because of
> > checkpatch warning).
> 
> Sure, it doesnt complicate anything - Minoru, this is the version to go
> for.
> 
> > Alas, I still think we don't need to change so much in -stable to
> > fix the cls_cgroup oops, so I attach a patch which I think is
> > enough for -stable and probably -net too. It could be "reverted"
> > in -net-next just after applying cls_api patch. Of course, treat
> > it only as my humble proposal, and feel free to recommend to David
> > your version, no problem (really).
> 
> 
> My view is the same - that the second patch doesnt fix the root
> cause; and it is not that complicated to fix the root cause. So I humbly
> disagree with you.
> 
> The issue is that a classifer (cls_group in this example) is being
> misconfigured. It rejects the config - but the tp has already been
> added. 
> It then tries to use the tp in the fast and fails. 
> 
> If you look as closely as you did with the patch i posted, youd find
> ways to construct similar hostile misconfigs for other classifiers.
> You just need to create the scenario where the attributes will fail to
> validate. 
> 
> I actually suspect the most common scenario for such a failure is not
> that head is null (I doubt in Minoru case that allocation will fail);
> rather it is some reference to head->something.

The patch #2 is obviously worse and fixes less (of course it still
needs testing for Minoru's case), but I'm 100% confident it can't
introduce any regression (neither take 1 nor 2), which is much harder
to say about patch #1, considering various "rude" configs we could
miss (but we could give them some time to show off). But, as I've
written before, I'm really (really) OK with your decision.

Cheers,
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
Minoru Usui June 2, 2009, 6:59 a.m. UTC | #3 
On Mon, 01 Jun 2009 18:06:03 -0400
jamal <hadi@cyberus.ca> wrote:

> On Mon, 2009-06-01 at 22:49 +0200, Jarek Poplawski wrote:
> 
> > 
> > Actually, I'd insist with the old rock and handling that other rude
> > u32 case, at least until it's fixed in place. So I attach my version
> > of your patch (additionally I removed a pair of braces because of
> > checkpatch warning).
> 
> Sure, it doesnt complicate anything - Minoru, this is the version to go
> for.

I've tested Jarek's patch #1(for cls_api.c).
It works fine to me. Thanks Jarek.

	Tested-by: Minoru Usui <usui@mxm.nes.nec.co.jp>

If we have to test Jarek's patch #2, I'll test it tomorrow.
What do you think Jamal?
Jarek Poplawski June 2, 2009, 7:24 a.m. UTC | #4 
On Tue, Jun 02, 2009 at 03:59:19PM +0900, Minoru Usui wrote:
> On Mon, 01 Jun 2009 18:06:03 -0400
> jamal <hadi@cyberus.ca> wrote:
> 
> > On Mon, 2009-06-01 at 22:49 +0200, Jarek Poplawski wrote:
> > 
> > > 
> > > Actually, I'd insist with the old rock and handling that other rude
> > > u32 case, at least until it's fixed in place. So I attach my version
> > > of your patch (additionally I removed a pair of braces because of
> > > checkpatch warning).
> > 
> > Sure, it doesnt complicate anything - Minoru, this is the version to go
> > for.
> 
> I've tested Jarek's patch #1(for cls_api.c).
> It works fine to me. Thanks Jarek.
> 
> 	Tested-by: Minoru Usui <usui@mxm.nes.nec.co.jp>

Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

Actually, as far as I can remember, it's your patch with slight
modifications by Jamal and me. So please Minoru, resend this patch
as Jamal previously suggested: with your changelog and Jamal's,
plus maybe mine (if you don't mind) signed-off-by's.

Thanks,
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
Minoru Usui June 2, 2009, 8:29 a.m. UTC | #5 
On Tue, 2 Jun 2009 07:24:36 +0000
Jarek Poplawski <jarkao2@gmail.com> wrote:

> On Tue, Jun 02, 2009 at 03:59:19PM +0900, Minoru Usui wrote:
> > On Mon, 01 Jun 2009 18:06:03 -0400
> > jamal <hadi@cyberus.ca> wrote:
> > 
> > > On Mon, 2009-06-01 at 22:49 +0200, Jarek Poplawski wrote:
> > > 
> > > > 
> > > > Actually, I'd insist with the old rock and handling that other rude
> > > > u32 case, at least until it's fixed in place. So I attach my version
> > > > of your patch (additionally I removed a pair of braces because of
> > > > checkpatch warning).
> > > 
> > > Sure, it doesnt complicate anything - Minoru, this is the version to go
> > > for.
> > 
> > I've tested Jarek's patch #1(for cls_api.c).
> > It works fine to me. Thanks Jarek.
> > 
> > 	Tested-by: Minoru Usui <usui@mxm.nes.nec.co.jp>
> 
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> 
> Actually, as far as I can remember, it's your patch with slight
> modifications by Jamal and me. So please Minoru, resend this patch
> as Jamal previously suggested: with your changelog and Jamal's,
> plus maybe mine (if you don't mind) signed-off-by's.
> 
> Thanks,
> Jarek P.

OK. Thank you for your advise.
I'll resend the patch as a new thread.
jamal June 2, 2009, 1:19 p.m. UTC | #6 
On Tue, 2009-06-02 at 15:59 +0900, Minoru Usui wrote:

> If we have to test Jarek's patch #2, I'll test it tomorrow.
> What do you think Jamal?

Yes please - even if it is for reasons of givein Jarek some peace of
mind;->
If something goes seriously wrong with other classifiers because of the
patch, and Jareks patch works, we can back out the current patch and fix
it with that approach.

cheers,
jamal


--
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
Minoru Usui June 4, 2009, 4:41 a.m. UTC | #7 
On Tue, 02 Jun 2009 09:19:25 -0400
jamal <hadi@cyberus.ca> wrote:

> On Tue, 2009-06-02 at 15:59 +0900, Minoru Usui wrote:
> 
> > If we have to test Jarek's patch #2, I'll test it tomorrow.
> > What do you think Jamal?
> 
> Yes please - even if it is for reasons of givein Jarek some peace of
> mind;->
> If something goes seriously wrong with other classifiers because of the
> patch, and Jareks patch works, we can back out the current patch and fix
> it with that approach.

I've tested Jarek's patch #2 without patch #1.
It works fine, too. Thanks Jarek.

	Tested-by: Minoru Usui <usui@mxm.nes.nec.co.jp>
Jarek Poplawski June 4, 2009, 6:34 a.m. UTC | #8 
On Thu, Jun 04, 2009 at 01:41:33PM +0900, Minoru Usui wrote:
> On Tue, 02 Jun 2009 09:19:25 -0400
> jamal <hadi@cyberus.ca> wrote:
> 
> > On Tue, 2009-06-02 at 15:59 +0900, Minoru Usui wrote:
> > 
> > > If we have to test Jarek's patch #2, I'll test it tomorrow.
> > > What do you think Jamal?
> > 
> > Yes please - even if it is for reasons of givein Jarek some peace of
> > mind;->
> > If something goes seriously wrong with other classifiers because of the
> > patch, and Jareks patch works, we can back out the current patch and fix
> > it with that approach.
> 
> I've tested Jarek's patch #2 without patch #1.
> It works fine, too. Thanks Jarek.
> 
> 	Tested-by: Minoru Usui <usui@mxm.nes.nec.co.jp>

Thank you very much. So, as Jamal wrote, let's wait - hoping it won't
be needed...

Jarek P.

Btw., it seems your box needs time update:

Subject: Re: [BUG] net_cls: Panic occured when net_cls subsystem use
Date: Thu, 4 Jun 2009 13:41:33 +0900
...
Original-Received: from mail03.kamome.nec.co.jp (mail03.kamome.nec.co.jp [10.25.43.7])
	by mailsv.nec.co.jp (8.13.8/8.13.4) with ESMTP id n5455gP4006057;
	Thu, 4 Jun 2009 14:06:27 +0900 (JST)
--
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 June 4, 2009, 8:37 a.m. UTC | #9 
On Thu, Jun 04, 2009 at 06:34:45AM +0000, Jarek Poplawski wrote:
...
> Btw., it seems your box needs time update:

Or rather there was some other delay. I shouldn't bother...

Sorry,
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
diff mbox

Patch

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 0759f32..09cdcdf 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -135,6 +135,7 @@  static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n, void *arg)
 	unsigned long cl;
 	unsigned long fh;
 	int err;
+	int tp_created = 0;
 
 	if (net != &init_net)
 		return -EINVAL;
@@ -266,10 +267,7 @@  replay:
 			goto errout;
 		}
 
-		spin_lock_bh(root_lock);
-		tp->next = *back;
-		*back = tp;
-		spin_unlock_bh(root_lock);
+		tp_created = 1;
 
 	} else if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], tp->ops->kind))
 		goto errout;
@@ -296,8 +294,11 @@  replay:
 		switch (n->nlmsg_type) {
 		case RTM_NEWTFILTER:
 			err = -EEXIST;
-			if (n->nlmsg_flags & NLM_F_EXCL)
+			if (n->nlmsg_flags & NLM_F_EXCL) {
+				if (tp_created)
+					tcf_destroy(tp);
 				goto errout;
+			}
 			break;
 		case RTM_DELTFILTER:
 			err = tp->ops->delete(tp, fh);
@@ -314,8 +315,18 @@  replay:
 	}
 
 	err = tp->ops->change(tp, cl, t->tcm_handle, tca, &fh);
-	if (err == 0)
+	if (err == 0) {
+		if (tp_created) {
+			spin_lock_bh(root_lock);
+			tp->next = *back;
+			*back = tp;
+			spin_unlock_bh(root_lock);
+		}
 		tfilter_notify(skb, n, tp, fh, RTM_NEWTFILTER);
+	} else {
+		if (tp_created)
+			tcf_destroy(tp);
+	}
 
 errout:
 	if (cl)

--------------------------> patch #2

 net/sched/cls_cgroup.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 1ab4542..3b2026f 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -101,6 +101,8 @@  static int cls_cgroup_classify(struct sk_buff *skb, struct tcf_proto *tp,
 	struct cgroup_cls_state *cs;
 	int ret = 0;
 
+	if (!head)
+		return -1;
 	/*
 	 * Due to the nature of the classifier it is required to ignore all
 	 * packets originating from softirq context as accessing `current'