| Message ID | 50D04B4B.7060002@linux-ipv6.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
YOSHIFUJI Hideaki wrote: > Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org> > --- > net/ipv6/ndisc.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c > index a181113..0a4f3a9 100644 > --- a/net/ipv6/ndisc.c > +++ b/net/ipv6/ndisc.c > @@ -1332,6 +1332,19 @@ static void ndisc_redirect_rcv(struct sk_buff *skb) > icmpv6_notify(skb, NDISC_REDIRECT, 0, 0); > } > > +static u8 *ndisc_fill_redirect_hdr_option(u8 *opt, struct sk_buff *orig_skb, > + int rd_len) > +{ > + memset(opt, 0, 8); > + *(opt++) = ND_OPT_REDIRECT_HDR; > + *(opt++) = (rd_len >> 3); > + opt += 6; > + > + memcpy(opt, ipv6_hdr(orig_skb), rd_len - 8); > + > + return opt; > +} > + > void ndisc_send_redirect(struct sk_buff *skb, const struct in6_addr *target) > { > struct net_device *dev = skb->dev; By the way, is it really safe to use memcpy here? Should we use skb_copy_bits() instead? --yoshfuji -- 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
YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org> writes: > Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org> > --- > net/ipv6/ndisc.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c > index a181113..0a4f3a9 100644 > --- a/net/ipv6/ndisc.c > +++ b/net/ipv6/ndisc.c > @@ -1332,6 +1332,19 @@ static void ndisc_redirect_rcv(struct sk_buff *skb) > icmpv6_notify(skb, NDISC_REDIRECT, 0, 0); > } > > +static u8 *ndisc_fill_redirect_hdr_option(u8 *opt, struct sk_buff *orig_skb, > + int rd_len) > +{ > + memset(opt, 0, 8); > + *(opt++) = ND_OPT_REDIRECT_HDR; > + *(opt++) = (rd_len >> 3); > + opt += 6; > + > + memcpy(opt, ipv6_hdr(orig_skb), rd_len - 8); > + > + return opt; > +} > + I realize that it doesn't currently matter, but the above modification of "opt" looks like a bug-waiting-to-happen to me. > void ndisc_send_redirect(struct sk_buff *skb, const struct in6_addr *target) > { > struct net_device *dev = skb->dev; > @@ -1461,12 +1474,8 @@ void ndisc_send_redirect(struct sk_buff *skb, const struct in6_addr *target) > * build redirect option and copy skb over to the new packet. > */ > > - memset(opt, 0, 8); > - *(opt++) = ND_OPT_REDIRECT_HDR; > - *(opt++) = (rd_len >> 3); > - opt += 6; > - > - memcpy(opt, ipv6_hdr(skb), rd_len - 8); > + if (rd_len) > + opt = ndisc_fill_redirect_hdr_option(opt, skb, rd_len); I understand that opt isn't currently used after this, but if it ever is then it is going to come as big a surprise that this implies opt += 8; This was previously quite clear when the code was inline, but it becomes problematic when it is factored out. Bjørn -- 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
Bjørn Mork wrote: > YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org> writes: > >> Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org> >> --- >> net/ipv6/ndisc.c | 21 +++++++++++++++------ >> 1 file changed, 15 insertions(+), 6 deletions(-) >> >> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c >> index a181113..0a4f3a9 100644 >> --- a/net/ipv6/ndisc.c >> +++ b/net/ipv6/ndisc.c >> @@ -1332,6 +1332,19 @@ static void ndisc_redirect_rcv(struct sk_buff *skb) >> icmpv6_notify(skb, NDISC_REDIRECT, 0, 0); >> } >> >> +static u8 *ndisc_fill_redirect_hdr_option(u8 *opt, struct sk_buff *orig_skb, >> + int rd_len) >> +{ >> + memset(opt, 0, 8); >> + *(opt++) = ND_OPT_REDIRECT_HDR; >> + *(opt++) = (rd_len >> 3); >> + opt += 6; >> + >> + memcpy(opt, ipv6_hdr(orig_skb), rd_len - 8); >> + >> + return opt; >> +} >> + > > I realize that it doesn't currently matter, but the above modification > of "opt" looks like a bug-waiting-to-happen to me. > >> void ndisc_send_redirect(struct sk_buff *skb, const struct in6_addr *target) >> { >> struct net_device *dev = skb->dev; >> @@ -1461,12 +1474,8 @@ void ndisc_send_redirect(struct sk_buff *skb, const struct in6_addr *target) >> * build redirect option and copy skb over to the new packet. >> */ >> >> - memset(opt, 0, 8); >> - *(opt++) = ND_OPT_REDIRECT_HDR; >> - *(opt++) = (rd_len >> 3); >> - opt += 6; >> - >> - memcpy(opt, ipv6_hdr(skb), rd_len - 8); >> + if (rd_len) >> + opt = ndisc_fill_redirect_hdr_option(opt, skb, rd_len); > > > I understand that opt isn't currently used after this, but if it ever is > then it is going to come as big a surprise that this implies opt += 8; > > This was previously quite clear when the code was inline, but it becomes > problematic when it is factored out. I understand your concern. opt will be disappeared by following changeset (12 of 17). --yoshfuji -- 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
(2012年12月20日 01:25), YOSHIFUJI Hideaki wrote: > Bjørn Mork wrote: >> YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org> writes: >>> +static u8 *ndisc_fill_redirect_hdr_option(u8 *opt, struct sk_buff *orig_skb, >>> + int rd_len) >>> +{ >>> + memset(opt, 0, 8); >>> + *(opt++) = ND_OPT_REDIRECT_HDR; >>> + *(opt++) = (rd_len >> 3); >>> + opt += 6; >>> + >>> + memcpy(opt, ipv6_hdr(orig_skb), rd_len - 8); >>> + >>> + return opt; >>> +} : >> I understand that opt isn't currently used after this, but if it ever is >> then it is going to come as big a surprise that this implies opt += 8; >> >> This was previously quite clear when the code was inline, but it becomes >> problematic when it is factored out. > > I understand your concern. opt will be disappeared by following > changeset (12 of 17). Argh, I now notice return value was not quite right; it should return opt + rd_len - 8. Fixed in my local tree. Thanks. --yoshfuji -- 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/ipv6/ndisc.c b/net/ipv6/ndisc.c index a181113..0a4f3a9 100644 --- a/net/ipv6/ndisc.c +++ b/net/ipv6/ndisc.c @@ -1332,6 +1332,19 @@ static void ndisc_redirect_rcv(struct sk_buff *skb) icmpv6_notify(skb, NDISC_REDIRECT, 0, 0); } +static u8 *ndisc_fill_redirect_hdr_option(u8 *opt, struct sk_buff *orig_skb, + int rd_len) +{ + memset(opt, 0, 8); + *(opt++) = ND_OPT_REDIRECT_HDR; + *(opt++) = (rd_len >> 3); + opt += 6; + + memcpy(opt, ipv6_hdr(orig_skb), rd_len - 8); + + return opt; +} + void ndisc_send_redirect(struct sk_buff *skb, const struct in6_addr *target) { struct net_device *dev = skb->dev; @@ -1461,12 +1474,8 @@ void ndisc_send_redirect(struct sk_buff *skb, const struct in6_addr *target) * build redirect option and copy skb over to the new packet. */ - memset(opt, 0, 8); - *(opt++) = ND_OPT_REDIRECT_HDR; - *(opt++) = (rd_len >> 3); - opt += 6; - - memcpy(opt, ipv6_hdr(skb), rd_len - 8); + if (rd_len) + opt = ndisc_fill_redirect_hdr_option(opt, skb, rd_len); msg->icmph.icmp6_cksum = csum_ipv6_magic(&saddr_buf, &ipv6_hdr(skb)->saddr, len, IPPROTO_ICMPV6,
Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org> --- net/ipv6/ndisc.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)