[patch net-next 1/2] net: sched: cls_basic: fix error path in basic_change()

[patch net-next 1/2] net: sched: cls_basic: fix error path in basic_change()

[Jiri Pirko]

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 net/sched/cls_basic.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index 7cf0a62..5aed341 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -178,10 +178,9 @@ static int basic_change(struct net *net, struct sk_buff *in_skb,
 			return -EINVAL;
 	}
 
-	err = -ENOBUFS;
 	fnew = kzalloc(sizeof(*fnew), GFP_KERNEL);
-	if (fnew == NULL)
-		goto errout;
+	if (!fnew)
+		return -ENOBUFS;
 
 	tcf_exts_init(&fnew->exts, TCA_BASIC_ACT, TCA_BASIC_POLICE);
 	err = -EINVAL;
-- 
1.9.3

[patch net-next 2/2] net: sched: cls: use nla_nest_cancel instead of nlmsg_trim

[Jiri Pirko]

To cancel nesting, this function is more convenient.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 net/sched/cls_cgroup.c  | 3 +--
 net/sched/cls_flow.c    | 2 +-
 net/sched/cls_fw.c      | 3 +--
 net/sched/cls_route.c   | 3 +--
 net/sched/cls_rsvp.h    | 3 +--
 net/sched/cls_tcindex.c | 3 +--
 6 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 741bfa7..221697a 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -177,7 +177,6 @@ static int cls_cgroup_dump(struct net *net, struct tcf_proto *tp, unsigned long
 			   struct sk_buff *skb, struct tcmsg *t)
 {
 	struct cls_cgroup_head *head = rtnl_dereference(tp->root);
-	unsigned char *b = skb_tail_pointer(skb);
 	struct nlattr *nest;
 
 	t->tcm_handle = head->handle;
@@ -198,7 +197,7 @@ static int cls_cgroup_dump(struct net *net, struct tcf_proto *tp, unsigned long
 	return skb->len;
 
 nla_put_failure:
-	nlmsg_trim(skb, b);
+	nla_nest_cancel(skb, nest);
 	return -1;
 }
 
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index 8e22718..15d68f2 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -638,7 +638,7 @@ static int flow_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 	return skb->len;
 
 nla_put_failure:
-	nlmsg_trim(skb, nest);
+	nla_nest_cancel(skb, nest);
 	return -1;
 }
 
diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index 23fda2a..a5269f7 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -356,7 +356,6 @@ static int fw_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 {
 	struct fw_head *head = rtnl_dereference(tp->root);
 	struct fw_filter *f = (struct fw_filter *)fh;
-	unsigned char *b = skb_tail_pointer(skb);
 	struct nlattr *nest;
 
 	if (f == NULL)
@@ -397,7 +396,7 @@ static int fw_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 	return skb->len;
 
 nla_put_failure:
-	nlmsg_trim(skb, b);
+	nla_nest_cancel(skb, nest);
 	return -1;
 }
 
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index 098a273..2ecd246 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -593,7 +593,6 @@ static int route4_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 		       struct sk_buff *skb, struct tcmsg *t)
 {
 	struct route4_filter *f = (struct route4_filter *)fh;
-	unsigned char *b = skb_tail_pointer(skb);
 	struct nlattr *nest;
 	u32 id;
 
@@ -635,7 +634,7 @@ static int route4_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 	return skb->len;
 
 nla_put_failure:
-	nlmsg_trim(skb, b);
+	nla_nest_cancel(skb, nest);
 	return -1;
 }
 
diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
index b7af362..edd8ade 100644
--- a/net/sched/cls_rsvp.h
+++ b/net/sched/cls_rsvp.h
@@ -653,7 +653,6 @@ static int rsvp_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 {
 	struct rsvp_filter *f = (struct rsvp_filter *)fh;
 	struct rsvp_session *s;
-	unsigned char *b = skb_tail_pointer(skb);
 	struct nlattr *nest;
 	struct tc_rsvp_pinfo pinfo;
 
@@ -694,7 +693,7 @@ static int rsvp_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 	return skb->len;
 
 nla_put_failure:
-	nlmsg_trim(skb, b);
+	nla_nest_cancel(skb, nest);
 	return -1;
 }
 
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 0d9d891..48d5e7b 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -489,7 +489,6 @@ static int tcindex_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 {
 	struct tcindex_data *p = rtnl_dereference(tp->root);
 	struct tcindex_filter_result *r = (struct tcindex_filter_result *) fh;
-	unsigned char *b = skb_tail_pointer(skb);
 	struct nlattr *nest;
 
 	pr_debug("tcindex_dump(tp %p,fh 0x%lx,skb %p,t %p),p %p,r %p,b %p\n",
@@ -543,7 +542,7 @@ static int tcindex_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 	return skb->len;
 
 nla_put_failure:
-	nlmsg_trim(skb, b);
+	nla_nest_cancel(skb, nest);
 	return -1;
 }
 
-- 
1.9.3

Re: [patch net-next 1/2] net: sched: cls_basic: fix error path in basic_change()

[John Fastabend]

On 12/05/2014 06:50 AM, Jiri Pirko wrote:
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
>   net/sched/cls_basic.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
> index 7cf0a62..5aed341 100644
> --- a/net/sched/cls_basic.c
> +++ b/net/sched/cls_basic.c
> @@ -178,10 +178,9 @@ static int basic_change(struct net *net, struct sk_buff *in_skb,
>   			return -EINVAL;
>   	}
>
> -	err = -ENOBUFS;
>   	fnew = kzalloc(sizeof(*fnew), GFP_KERNEL);
> -	if (fnew == NULL)
> -		goto errout;
> +	if (!fnew)
> +		return -ENOBUFS;
>
>   	tcf_exts_init(&fnew->exts, TCA_BASIC_ACT, TCA_BASIC_POLICE);
>   	err = -EINVAL;
>

Nice catch, thanks!

Reviewed-by: John Fastabend <john.r.fastabend@intel.com>

-- 
John Fastabend         Intel Corporation

Re: [patch net-next 1/2] net: sched: cls_basic: fix error path in basic_change()

[Eric Dumazet]

On Fri, 2014-12-05 at 07:29 -0800, John Fastabend wrote:
> On 12/05/2014 06:50 AM, Jiri Pirko wrote:
> > Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> > ---
> >   net/sched/cls_basic.c | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
> > index 7cf0a62..5aed341 100644
> > --- a/net/sched/cls_basic.c
> > +++ b/net/sched/cls_basic.c
> > @@ -178,10 +178,9 @@ static int basic_change(struct net *net, struct sk_buff *in_skb,
> >   			return -EINVAL;
> >   	}
> >
> > -	err = -ENOBUFS;
> >   	fnew = kzalloc(sizeof(*fnew), GFP_KERNEL);
> > -	if (fnew == NULL)
> > -		goto errout;
> > +	if (!fnew)
> > +		return -ENOBUFS;
> >
> >   	tcf_exts_init(&fnew->exts, TCA_BASIC_ACT, TCA_BASIC_POLICE);
> >   	err = -EINVAL;
> >
> 
> Nice catch, thanks!
> 
> Reviewed-by: John Fastabend <john.r.fastabend@intel.com>

Sorry, but this looks a cosmetic change, right ?

If it is a fix, we'd like a 'Fixes: ...' tag.

Re: [patch net-next 1/2] net: sched: cls_basic: fix error path in basic_change()

[John Fastabend]

On 12/05/2014 08:03 AM, Eric Dumazet wrote:
> On Fri, 2014-12-05 at 07:29 -0800, John Fastabend wrote:
>> On 12/05/2014 06:50 AM, Jiri Pirko wrote:
>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>> ---
>>>    net/sched/cls_basic.c | 5 ++---
>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
>>> index 7cf0a62..5aed341 100644
>>> --- a/net/sched/cls_basic.c
>>> +++ b/net/sched/cls_basic.c
>>> @@ -178,10 +178,9 @@ static int basic_change(struct net *net, struct sk_buff *in_skb,
>>>    			return -EINVAL;
>>>    	}
>>>
>>> -	err = -ENOBUFS;
>>>    	fnew = kzalloc(sizeof(*fnew), GFP_KERNEL);
>>> -	if (fnew == NULL)
>>> -		goto errout;
>>> +	if (!fnew)
>>> +		return -ENOBUFS;
>>>
>>>    	tcf_exts_init(&fnew->exts, TCA_BASIC_ACT, TCA_BASIC_POLICE);
>>>    	err = -EINVAL;
>>>
>>
>> Nice catch, thanks!
>>
>> Reviewed-by: John Fastabend <john.r.fastabend@intel.com>
>
> Sorry, but this looks a cosmetic change, right ?
>
> If it is a fix, we'd like a 'Fixes: ...' tag.
>
>
>

oops, you are right. fnew is null, free'ing the null value
is no issue at all from the error path. And if we are going
to start converting the use of

	if (foo == NULL)
to
	if (!foo)

in ./net/sched that is OK but not really worth the noise in
my opinion there are a lot of these in the code for quiet
sometime.

Sorry for the noise. I misread it as fixing a free on some
non-null value, which is not the case.

.John


-- 
John Fastabend         Intel Corporation

Re: [patch net-next 1/2] net: sched: cls_basic: fix error path in basic_change()

[Sergei Shtylyov]

Hello.

On 12/05/2014 05:50 PM, Jiri Pirko wrote:

> Signed-off-by: Jiri Pirko <jiri@resnulli.us>

    Care to explain what's wrong?

> ---
>   net/sched/cls_basic.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)

> diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
> index 7cf0a62..5aed341 100644
> --- a/net/sched/cls_basic.c
> +++ b/net/sched/cls_basic.c
> @@ -178,10 +178,9 @@ static int basic_change(struct net *net, struct sk_buff *in_skb,
>   			return -EINVAL;
>   	}
>
> -	err = -ENOBUFS;
>   	fnew = kzalloc(sizeof(*fnew), GFP_KERNEL);
> -	if (fnew == NULL)
> -		goto errout;
> +	if (!fnew)
> +		return -ENOBUFS;
>
>   	tcf_exts_init(&fnew->exts, TCA_BASIC_ACT, TCA_BASIC_POLICE);
>   	err = -EINVAL;

WBR, Sergei

Re: [patch net-next 1/2] net: sched: cls_basic: fix error path in basic_change()

[Jiri Pirko]

Fri, Dec 05, 2014 at 05:03:51PM CET, eric.dumazet@gmail.com wrote:
>On Fri, 2014-12-05 at 07:29 -0800, John Fastabend wrote:
>> On 12/05/2014 06:50 AM, Jiri Pirko wrote:
>> > Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> > ---
>> >   net/sched/cls_basic.c | 5 ++---
>> >   1 file changed, 2 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
>> > index 7cf0a62..5aed341 100644
>> > --- a/net/sched/cls_basic.c
>> > +++ b/net/sched/cls_basic.c
>> > @@ -178,10 +178,9 @@ static int basic_change(struct net *net, struct sk_buff *in_skb,
>> >   			return -EINVAL;
>> >   	}
>> >
>> > -	err = -ENOBUFS;
>> >   	fnew = kzalloc(sizeof(*fnew), GFP_KERNEL);
>> > -	if (fnew == NULL)
>> > -		goto errout;
>> > +	if (!fnew)
>> > +		return -ENOBUFS;
>> >
>> >   	tcf_exts_init(&fnew->exts, TCA_BASIC_ACT, TCA_BASIC_POLICE);
>> >   	err = -EINVAL;
>> >
>> 
>> Nice catch, thanks!
>> 
>> Reviewed-by: John Fastabend <john.r.fastabend@intel.com>
>
>Sorry, but this looks a cosmetic change, right ?
>
>If it is a fix, we'd like a 'Fixes: ...' tag.

Yep, cosmetic, no need to go to kfree in case kzalloc fails.

Re: [patch net-next 1/2] net: sched: cls_basic: fix error path in basic_change()

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Fri,  5 Dec 2014 15:50:22 +0100

> Signed-off-by: Jiri Pirko <jiri@resnulli.us>

Applied.

Re: [patch net-next 2/2] net: sched: cls: use nla_nest_cancel instead of nlmsg_trim

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Fri,  5 Dec 2014 15:50:23 +0100

> To cancel nesting, this function is more convenient.
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>

Applied.

Re: [patch net-next 2/2] net: sched: cls: use nla_nest_cancel instead of nlmsg_trim

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Tue, 09 Dec 2014 15:42:18 -0500 (EST)

> From: Jiri Pirko <jiri@resnulli.us>
> Date: Fri,  5 Dec 2014 15:50:23 +0100
> 
>> To cancel nesting, this function is more convenient.
>> 
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> 
> Applied.

This doesn't even compile, when you took away the local 'b' variable
it is still referenced by pr_debug() calls.

[patch net-next v2] net: sched: cls: use nla_nest_cancel instead of nlmsg_trim

[Jiri Pirko]

To cancel nesting, this function is more convenient.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
v1->v2:
- removed remaining use of b in tcindex pointed out by DaveM
---
 net/sched/cls_cgroup.c  | 3 +--
 net/sched/cls_flow.c    | 2 +-
 net/sched/cls_fw.c      | 3 +--
 net/sched/cls_route.c   | 3 +--
 net/sched/cls_rsvp.h    | 3 +--
 net/sched/cls_tcindex.c | 7 +++----
 6 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 741bfa7..221697a 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -177,7 +177,6 @@ static int cls_cgroup_dump(struct net *net, struct tcf_proto *tp, unsigned long
 			   struct sk_buff *skb, struct tcmsg *t)
 {
 	struct cls_cgroup_head *head = rtnl_dereference(tp->root);
-	unsigned char *b = skb_tail_pointer(skb);
 	struct nlattr *nest;
 
 	t->tcm_handle = head->handle;
@@ -198,7 +197,7 @@ static int cls_cgroup_dump(struct net *net, struct tcf_proto *tp, unsigned long
 	return skb->len;
 
 nla_put_failure:
-	nlmsg_trim(skb, b);
+	nla_nest_cancel(skb, nest);
 	return -1;
 }
 
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index 8e22718..15d68f2 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -638,7 +638,7 @@ static int flow_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 	return skb->len;
 
 nla_put_failure:
-	nlmsg_trim(skb, nest);
+	nla_nest_cancel(skb, nest);
 	return -1;
 }
 
diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index 23fda2a..a5269f7 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -356,7 +356,6 @@ static int fw_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 {
 	struct fw_head *head = rtnl_dereference(tp->root);
 	struct fw_filter *f = (struct fw_filter *)fh;
-	unsigned char *b = skb_tail_pointer(skb);
 	struct nlattr *nest;
 
 	if (f == NULL)
@@ -397,7 +396,7 @@ static int fw_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 	return skb->len;
 
 nla_put_failure:
-	nlmsg_trim(skb, b);
+	nla_nest_cancel(skb, nest);
 	return -1;
 }
 
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index 098a273..2ecd246 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -593,7 +593,6 @@ static int route4_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 		       struct sk_buff *skb, struct tcmsg *t)
 {
 	struct route4_filter *f = (struct route4_filter *)fh;
-	unsigned char *b = skb_tail_pointer(skb);
 	struct nlattr *nest;
 	u32 id;
 
@@ -635,7 +634,7 @@ static int route4_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 	return skb->len;
 
 nla_put_failure:
-	nlmsg_trim(skb, b);
+	nla_nest_cancel(skb, nest);
 	return -1;
 }
 
diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
index b7af362..edd8ade 100644
--- a/net/sched/cls_rsvp.h
+++ b/net/sched/cls_rsvp.h
@@ -653,7 +653,6 @@ static int rsvp_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 {
 	struct rsvp_filter *f = (struct rsvp_filter *)fh;
 	struct rsvp_session *s;
-	unsigned char *b = skb_tail_pointer(skb);
 	struct nlattr *nest;
 	struct tc_rsvp_pinfo pinfo;
 
@@ -694,7 +693,7 @@ static int rsvp_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 	return skb->len;
 
 nla_put_failure:
-	nlmsg_trim(skb, b);
+	nla_nest_cancel(skb, nest);
 	return -1;
 }
 
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 0d9d891..bd49bf5 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -489,11 +489,10 @@ static int tcindex_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 {
 	struct tcindex_data *p = rtnl_dereference(tp->root);
 	struct tcindex_filter_result *r = (struct tcindex_filter_result *) fh;
-	unsigned char *b = skb_tail_pointer(skb);
 	struct nlattr *nest;
 
-	pr_debug("tcindex_dump(tp %p,fh 0x%lx,skb %p,t %p),p %p,r %p,b %p\n",
-		 tp, fh, skb, t, p, r, b);
+	pr_debug("tcindex_dump(tp %p,fh 0x%lx,skb %p,t %p),p %p,r %p\n",
+		 tp, fh, skb, t, p, r);
 	pr_debug("p->perfect %p p->h %p\n", p->perfect, p->h);
 
 	nest = nla_nest_start(skb, TCA_OPTIONS);
@@ -543,7 +542,7 @@ static int tcindex_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 	return skb->len;
 
 nla_put_failure:
-	nlmsg_trim(skb, b);
+	nla_nest_cancel(skb, nest);
 	return -1;
 }
 
-- 
1.9.3

Re: [patch net-next v2] net: sched: cls: use nla_nest_cancel instead of nlmsg_trim

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Tue,  9 Dec 2014 22:23:29 +0100

> To cancel nesting, this function is more convenient.
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
> v1->v2:
> - removed remaining use of b in tcindex pointed out by DaveM

This one compiles! :-)

Applied, thanks.