[RFC] networking: convert many more places to skb_put_zero()

Hi, 
Indeed, it find more.
Compare with my patch, still lost pattern like below,
1. sctp and openvswitch
 	kfree_skb(skb);
 
 	if (padlen) {
-		u8 *pad = skb_put(nskb, padlen);
-		memset(pad, 0, padlen);
+		skb_put_zero(nskb, padlen);

I will send a separate patch for ipv6/ndisc.c once spatch done.



> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Johannes Berg
> Sent: Thursday, June 15, 2017 4:18 AM
> To: netdev@vger.kernel.org
> Cc: Johannes Berg
> Subject: [RFC] networking: convert many more places to skb_put_zero()
> 
> From: Johannes Berg <johannes.berg@intel.com>
> 
> There were many places that my previous spatch didn't find,
> as pointed out by yuan linyu in various patches.
> 
> The following spatch found many more and also removes the
> now unnecessary casts:
> 
>     @@
>     identifier p, p2;
>     expression len;
>     expression skb;
>     type t, t2;
>     @@
>     (
>     -p = skb_put(skb, len);
>     +p = skb_put_zero(skb, len);
>     |
>     -p = (t)skb_put(skb, len);
>     +p = skb_put_zero(skb, len);
>     )
>     (
>     p2 = (t2)p;
>     -memset(p2, 0, len);
>     |
>     -memset(p, 0, len);
>     )
> 
>     @@
>     type t, t2;
>     identifier p, p2;
>     expression skb;
>     @@
>     t *p;
>     ...
>     (
>     -p = skb_put(skb, sizeof(t));
>     +p = skb_put_zero(skb, sizeof(t));
>     |
>     -p = (t *)skb_put(skb, sizeof(t));
>     +p = skb_put_zero(skb, sizeof(t));
>     )
>     (
>     p2 = (t2)p;
>     -memset(p2, 0, sizeof(*p));
>     |
>     -memset(p, 0, sizeof(*p));
>     )
>

On Thu, 2017-06-15 at 00:23 +0000, YUAN Linyu wrote:
> Hi, 
> Indeed, it find more.
> Compare with my patch, still lost pattern like below,
> 1. sctp and openvswitch
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -463,7 +463,7 @@ static int sctp_packet_pack(struct sctp_packet
> *packet,
>  
>  			padding = SCTP_PAD4(chunk->skb->len) -
> chunk->skb->len;
>  			if (padding)
> -				memset(skb_put(chunk->skb, padding),
> 0, padding);
> +				skb_put_zero(chunk->skb, padding);

Yep, good catch, this finds 18 instances thereof:

@@
expression skb, len;
@@
-memset(skb_put(skb, len), 0, len);
+skb_put_zero(skb, len);


> --- a/net/dsa/tag_trailer.c
> +++ b/net/dsa/tag_trailer.c
> @@ -43,8 +43,7 @@ static struct sk_buff *trailer_xmit(struct sk_buff
> *skb, struct net_device *dev)
>  	kfree_skb(skb);
>  
>  	if (padlen) {
> -		u8 *pad = skb_put(nskb, padlen);
> -		memset(pad, 0, padlen);
> +		skb_put_zero(nskb, padlen);

I'd have thought it finds this, but indeed it doesn't; there's only one
instances this changes though:

@@
type t;
expression skb, len;
identifier p;
@@
t *p
- = skb_put(skb, len);
+ = skb_put_zero(skb, len);
-memset(p, 0, len);

and it can't figure out that it should remove the variable, without
much more work that's not really worth it for one instance :)

johannes

> -----Original Message-----

> From: Johannes Berg [mailto:johannes@sipsolutions.net]

> Sent: Thursday, June 15, 2017 2:58 PM

> To: YUAN Linyu; netdev@vger.kernel.org

> Subject: Re: [RFC] networking: convert many more places to skb_put_zero()

> 

> On Thu, 2017-06-15 at 00:23 +0000, YUAN Linyu wrote:

> > Hi,

> > Indeed, it find more.

> > Compare with my patch, still lost pattern like below,

> > 1. sctp and openvswitch

> > --- a/net/sctp/output.c

> > +++ b/net/sctp/output.c

> > @@ -463,7 +463,7 @@ static int sctp_packet_pack(struct sctp_packet

> > *packet,

> >

> >  			padding = SCTP_PAD4(chunk->skb->len) -

> > chunk->skb->len;

> >  			if (padding)

> > -				memset(skb_put(chunk->skb, padding),

> > 0, padding);

> > +				skb_put_zero(chunk->skb, padding);

> 

> Yep, good catch, this finds 18 instances thereof:

> 

> @@

> expression skb, len;

> @@

> -memset(skb_put(skb, len), 0, len);

> +skb_put_zero(skb, len);

> 

> 

> > --- a/net/dsa/tag_trailer.c

> > +++ b/net/dsa/tag_trailer.c

> > @@ -43,8 +43,7 @@ static struct sk_buff *trailer_xmit(struct sk_buff

> > *skb, struct net_device *dev)

> >  	kfree_skb(skb);

> >

> >  	if (padlen) {

> > -		u8 *pad = skb_put(nskb, padlen);

> > -		memset(pad, 0, padlen);

> > +		skb_put_zero(nskb, padlen);

> 

> I'd have thought it finds this, but indeed it doesn't; there's only one

> instances this changes though:

> 

> @@

> type t;

> expression skb, len;

> identifier p;

> @@

> t *p

> - = skb_put(skb, len);

> + = skb_put_zero(skb, len);

> -memset(p, 0, len);

> 

> and it can't figure out that it should remove the variable, without

> much more work that's not really worth it for one instance :)

Yes, I agree, 
it conflict with previous spatch which will keep "pad" variable, right?

I can do it by hand if spatch not work
> 

> johannes

On Thu, 2017-06-15 at 07:05 +0000, YUAN Linyu wrote:
> > @@
> > type t;
> > expression skb, len;
> > identifier p;
> > @@
> > t *p
> > - = skb_put(skb, len);
> > + = skb_put_zero(skb, len);
> > -memset(p, 0, len);
> > 
> > and it can't figure out that it should remove the variable, without
> > much more work that's not really worth it for one instance :)
> 
> Yes, I agree, 
> it conflict with previous spatch which will keep "pad" variable,
> right?
> 
> I can do it by hand if spatch not work

I could teach spatch, but it's usually faster (for me) to post-process
the spatch changes to remove the extra variable - in this case though,
it's just not worth it at all since there's just a single change and
you already have a separate patch :)

Btw, just made a patch to add and use "skb_put_data()", just doing a
memcpy() into the skb_put() area also has lots of users.

johannes

> -----Original Message-----

> From: Johannes Berg [mailto:johannes@sipsolutions.net]

> Sent: Thursday, June 15, 2017 3:12 PM

> To: YUAN Linyu; netdev@vger.kernel.org

> Subject: Re: [RFC] networking: convert many more places to skb_put_zero()

> 

> On Thu, 2017-06-15 at 07:05 +0000, YUAN Linyu wrote:

> > > @@

> > > type t;

> > > expression skb, len;

> > > identifier p;

> > > @@

> > > t *p

> > > - = skb_put(skb, len);

> > > + = skb_put_zero(skb, len);

> > > -memset(p, 0, len);

> > >

> > > and it can't figure out that it should remove the variable, without

> > > much more work that's not really worth it for one instance :)

> >

> > Yes, I agree,

> > it conflict with previous spatch which will keep "pad" variable,

> > right?

> >

> > I can do it by hand if spatch not work

> 

> I could teach spatch, but it's usually faster (for me) to post-process

> the spatch changes to remove the extra variable - in this case though,

> it's just not worth it at all since there's just a single change and

> you already have a separate patch :)

In my opinion if spatch can do it even it found one place, keep it.
Only leave difficult places like ndisc.c to me.
> 

> Btw, just made a patch to add and use "skb_put_data()", just doing a

> memcpy() into the skb_put() area also has lots of users.

Yes, I also notice some places.
> 

> johannes

On Thu, 2017-06-15 at 07:20 +0000, YUAN Linyu wrote:
> > 
> In my opinion if spatch can do it even it found one place, keep it.
> Only leave difficult places like ndisc.c to me.

It's not so simple - I'd have to tailor the spatch to it pretty much I
guess, spending far more time on the spatch than the single place
warrants.

> > Btw, just made a patch to add and use "skb_put_data()", just doing
> > a
> > memcpy() into the skb_put() area also has lots of users.
> 
> Yes, I also notice some places.

I sent a patch.

johannes

Ok, understand 

> -----Original Message-----

> From: Johannes Berg [mailto:johannes@sipsolutions.net]

> Sent: Thursday, June 15, 2017 3:45 PM

> To: YUAN Linyu; netdev@vger.kernel.org

> Subject: Re: [RFC] networking: convert many more places to skb_put_zero()

> 

> On Thu, 2017-06-15 at 07:20 +0000, YUAN Linyu wrote:

> > >

> > In my opinion if spatch can do it even it found one place, keep it.

> > Only leave difficult places like ndisc.c to me.

> 

> It's not so simple - I'd have to tailor the spatch to it pretty much I

> guess, spending far more time on the spatch than the single place

> warrants.

> 

> > > Btw, just made a patch to add and use "skb_put_data()", just doing

> > > a

> > > memcpy() into the skb_put() area also has lots of users.

> >

> > Yes, I also notice some places.

> 

> I sent a patch.

> 

> johannes