Thanks Ying. I have used git to send the patch. Also, I have explicitly stated the name of message validation function in commit message.
For the reason of this change, Jon has explained. Thanks. Best regards, Tung Nguyen -----Original Message----- From: Ying Xue <[email protected]> Sent: Tuesday, April 24, 2018 7:22 PM To: tung quang nguyen <[email protected]>; [email protected] Subject: Re: [tipc-discussion] tipc: remove skb_clone() in function tipc_msg_extract() First of all, please submit your patch with git send email command. On 04/23/2018 11:16 AM, tung quang nguyen wrote: > Function tipc_msg_extract() is using skb_clone() to clone > > inner messages from bundle message. Although it is safe to > > use this method, it has an undesired effect that each clone > > inherits the true-size of the original buffer. As a result, > > the clone almost always has to be copied by the message > > validation function. Secondly, I cannot understand who is the message validation function. Can you please give us a bit detailed description about this? > > > > This commit fixes it by copy each inner message to a > > separately-allocated message buffer before being returned > > to caller. > > > > Signed-off-by: Tung Nguyen <[email protected]> > > --- > > net/tipc/msg.c | 35 +++++++++++++++-------------------- > > 1 file changed, 15 insertions(+), 20 deletions(-) > > > > diff --git a/net/tipc/msg.c b/net/tipc/msg.c > > index b6c45dc..b59c0e6 100644 > > --- a/net/tipc/msg.c > > +++ b/net/tipc/msg.c > > @@ -416,26 +416,31 @@ bool tipc_msg_bundle(struct sk_buff *skb, struct > tipc_msg *msg, u32 mtu) > > */ > > bool tipc_msg_extract(struct sk_buff *skb, struct sk_buff **iskb, int > *pos) > > { > > - struct tipc_msg *msg; > > - int imsz, offset; > > + struct tipc_msg *hdr, *ihdr; > > + int imsz; > > > > *iskb = NULL; > > if (unlikely(skb_linearize(skb))) > > goto none; > > > > - msg = buf_msg(skb); > > - offset = msg_hdr_sz(msg) + *pos; > > - if (unlikely(offset > (msg_size(msg) - MIN_H_SIZE))) > > + hdr = buf_msg(skb); > > + if (unlikely(*pos > (msg_data_sz(hdr) - MIN_H_SIZE))) > > goto none; > > > > - *iskb = skb_clone(skb, GFP_ATOMIC); > > - if (unlikely(!*iskb)) > > + ihdr = (struct tipc_msg *)(msg_data(hdr) + *pos); > > + imsz = msg_size(ihdr); > > + > > + if ((*pos + imsz) > msg_data_sz(hdr)) > > goto none; > > - skb_pull(*iskb, offset); > > - imsz = msg_size(buf_msg(*iskb)); > > - skb_trim(*iskb, imsz); > > + > > + *iskb = tipc_buf_acquire(imsz, GFP_ATOMIC); > > + if (!*iskb) > > + goto none; > > + > > + skb_copy_to_linear_data(*iskb, ihdr, imsz); In skb_clone(), only skb header is copied, by contrast, in your proposal, we have to copy the entire inner message to a new skb buffer. >From performance perspective, our performance will be worse. Can you please help me understand how your proposal is better than the current method? Thanks, Ying > > if (unlikely(!tipc_msg_validate(iskb))) > > goto none; > > + > > *pos += align(imsz); > > return true; > > none: > > @@ -531,12 +536,6 @@ bool tipc_msg_reverse(u32 own_node, struct > sk_buff **skb, int err) > > msg_set_hdr_sz(hdr, BASIC_H_SIZE); > > } > > > > - if (skb_cloned(_skb) && > > - pskb_expand_head(_skb, BUF_HEADROOM, BUF_TAILROOM, GFP_ATOMIC)) > > - goto exit; > > - > > - /* reassign after skb header modifications */ > > - hdr = buf_msg(_skb); > > /* Now reverse the concerned fields */ > > msg_set_errcode(hdr, err); > > msg_set_non_seq(hdr, 0); > > @@ -595,10 +594,6 @@ bool tipc_msg_lookup_dest(struct net *net, struct > sk_buff *skb, int *err) > > if (!skb_cloned(skb)) > > return true; > > > > - /* Unclone buffer in case it was bundled */ > > - if (pskb_expand_head(skb, BUF_HEADROOM, BUF_TAILROOM, GFP_ATOMIC)) > > - return false; > > - > > return true; > > } > > > > -- > > 2.1.4 > > > > -- > > Best regards, > > Tung Nguyen > > > > ---------------------------------------------------------------------- > -------- Check out the vibrant tech community on one of the world's > most engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > tipc-discussion mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/tipc-discussion > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ tipc-discussion mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion
