Message ID | 20090601204957.GA2760@ami.dom.local |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
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
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
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?
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
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.
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
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>
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
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 --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'