| Message ID | 1398235078-27088-3-git-send-email-lorenzo@google.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Le 22/04/2014 17:14, Lorenzo Colitti a écrit :> + > + if (sin6->sin6_family != AF_INET6) > + return -EAFNOSUPPORT; > + It has before returned -EINVAL, it changes the return to the user space. You made it consistent with other protocols, but perhaps should you add a notice in the commit changelog? > - if (sk->sk_bound_dev_if && > - sk->sk_bound_dev_if != u->sin6_scope_id) { > - return -EINVAL; > - } What about this check now ? Very good idea to simplify it, by the way. -- 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 Wed, Apr 23, 2014 at 8:11 PM, Florent Fourcot <florent.fourcot@enst-bretagne.fr> wrote: > Le 22/04/2014 17:14, Lorenzo Colitti a écrit :> + >> + if (sin6->sin6_family != AF_INET6) >> + return -EAFNOSUPPORT; >> + > > It has before returned -EINVAL, it changes the return to the user space. > You made it consistent with other protocols, but perhaps should you add > a notice in the commit changelog? Actually I'm not sure what the correct value is. When you setsockopt IPV6_V6ONLY and then send to a mapped address, the error you get depends on what you're trying to do - ip6_datagram_connect returns EAFNOSUPPORT, but udpv6_sendmsg, dccp_v6_connect and tcp_v6_connect return ENETUNREACH. I think EINVAL is wrong. EAFNOSUPPORT is probably best because the code doesn't support dual-stack ping sockets, but it could. There are probably very few users of this code at the moment, since the code was only released in 3.12, and support hasn't made it into iputils yet. And even there, ping just probably prints the error message and exits. So I don't think it's a big deal to change the return code. >> - if (sk->sk_bound_dev_if && >> - sk->sk_bound_dev_if != u->sin6_scope_id) { >> - return -EINVAL; >> - } > > What about this check now ? I think that was incorrect. It would return EINVAL even if you did something as simple as: - Open an IPv6 ping socket. - Bind it to eth0 with SO_BINDTODEVICE - Send a ping to 2001:: without specifying a scope id. -- 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 Wed, Apr 23, 2014 at 09:22:23PM +0900, Lorenzo Colitti wrote: > On Wed, Apr 23, 2014 at 8:11 PM, Florent Fourcot > <florent.fourcot@enst-bretagne.fr> wrote: > > Le 22/04/2014 17:14, Lorenzo Colitti a écrit :> + > >> + if (sin6->sin6_family != AF_INET6) > >> + return -EAFNOSUPPORT; > >> + > > > > It has before returned -EINVAL, it changes the return to the user space. > > You made it consistent with other protocols, but perhaps should you add > > a notice in the commit changelog? > > Actually I'm not sure what the correct value is. When you setsockopt > IPV6_V6ONLY and then send to a mapped address, the error you get > depends on what you're trying to do - ip6_datagram_connect returns > EAFNOSUPPORT, but udpv6_sendmsg, dccp_v6_connect and tcp_v6_connect > return ENETUNREACH. I think EINVAL is wrong. EAFNOSUPPORT is probably > best because the code doesn't support dual-stack ping sockets, but it > could. > > There are probably very few users of this code at the moment, since > the code was only released in 3.12, and support hasn't made it into > iputils yet. And even there, ping just probably prints the error > message and exits. So I don't think it's a big deal to change the > return code. Sure, but we don't know about other applications. Wouldn't it be just easier and leave this as is for now and finally let ipv6 ping sockets also handle ipv4? I looked at it some time ago and it didn't look complicated. > >> - if (sk->sk_bound_dev_if && > >> - sk->sk_bound_dev_if != u->sin6_scope_id) { > >> - return -EINVAL; > >> - } > > > > What about this check now ? > > I think that was incorrect. It would return EINVAL even if you did > something as simple as: > > - Open an IPv6 ping socket. > - Bind it to eth0 with SO_BINDTODEVICE > - Send a ping to 2001:: without specifying a scope id. Agree with that. Bye, Hannes -- 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 Fri, Apr 25, 2014 at 12:06 AM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: >> There are probably very few users of this code at the moment, since >> the code was only released in 3.12, and support hasn't made it into >> iputils yet. And even there, ping just probably prints the error >> message and exits. So I don't think it's a big deal to change the >> return code. > > Sure, but we don't know about other applications. Wouldn't it be just easier > and leave this as is for now and finally let ipv6 ping sockets also handle > ipv4? I looked at it some time ago and it didn't look complicated. I tried this today, but it's trickier than it looks. If you just naively call ping_v4_sendmsg, then things almost work, except the protocol field in the IPv4 header is set to 58 (ICMPv6) instead of 1 (ICMP). The reason is that we call ping_v4_push_pending_frames -> ip_push_pending_frames -> ip_finish_skb -> __ip_make_skb , which ends up doing: iph->protocol = sk->sk_protocol; Passing down the protocol all through that call chain seems overkill, because in basically every case except ICMP on a dual-stack socket, the protocol you want is in fact sk->sk_protocol. I suppose that since ping_v4_push_pending_frames gets called with the socket locked, we might be able to get away with setting sk->sk_protocol just before ping_v4_push_pending_frames and restoring it afterwards. But that feels hacky and I don't know what else it could break. -- 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 Fri, Apr 25, 2014 at 12:35 AM, Lorenzo Colitti <lorenzo@google.com> wrote: > I suppose that since ping_v4_push_pending_frames gets called with the > socket locked, we might be able to get away with setting > sk->sk_protocol just before ping_v4_push_pending_frames and restoring > it afterwards. But that feels hacky and I don't know what else it > could break. FWIW, here's what it looks like: http://patchwork.ozlabs.org/patch/342428/ -- 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/ping.c b/net/ipv6/ping.c index bda7429..96730c6 100644 --- a/net/ipv6/ping.c +++ b/net/ipv6/ping.c @@ -81,16 +81,17 @@ static int dummy_ipv6_chk_addr(struct net *net, const struct in6_addr *addr, int ping_v6_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg, size_t len) { + DECLARE_SOCKADDR(struct sockaddr_in6 *, sin6, msg->msg_name); struct inet_sock *inet = inet_sk(sk); - struct ipv6_pinfo *np = inet6_sk(sk); + struct ipv6_txoptions *opt, opt_space; struct icmp6hdr user_icmph; - int addr_type; + int addr_len = msg->msg_namelen; struct in6_addr *daddr; - int iif = 0; struct flowi6 fl6; int err; - int hlimit; - struct dst_entry *dst; + int hlimit, tclass, dontfrag; + int connected; + struct dst_entry *dst = NULL; struct rt6_info *rt; struct pingfakehdr pfh; @@ -101,63 +102,38 @@ int ping_v6_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg, if (err) return err; - if (msg->msg_name) { - DECLARE_SOCKADDR(struct sockaddr_in6 *, u, msg->msg_name); - if (msg->msg_namelen < sizeof(struct sockaddr_in6) || - u->sin6_family != AF_INET6) { + if (sin6) { + if (addr_len < sizeof(struct sockaddr_in6)) return -EINVAL; - } - if (sk->sk_bound_dev_if && - sk->sk_bound_dev_if != u->sin6_scope_id) { - return -EINVAL; - } - daddr = &(u->sin6_addr); - iif = u->sin6_scope_id; + + if (sin6->sin6_family != AF_INET6) + return -EAFNOSUPPORT; + + daddr = &sin6->sin6_addr; } else { - if (sk->sk_state != TCP_ESTABLISHED) - return -EDESTADDRREQ; daddr = &sk->sk_v6_daddr; } - if (!iif) - iif = sk->sk_bound_dev_if; - - addr_type = ipv6_addr_type(daddr); - if (__ipv6_addr_needs_scope_id(addr_type) && !iif) - return -EINVAL; - if (addr_type & IPV6_ADDR_MAPPED) + if (ipv6_addr_v4mapped(daddr)) return -EINVAL; - /* TODO: use ip6_datagram_send_ctl to get options from cmsg */ - memset(&fl6, 0, sizeof(fl6)); - fl6.flowi6_proto = IPPROTO_ICMPV6; - fl6.saddr = np->saddr; - fl6.daddr = *daddr; - fl6.flowi6_mark = sk->sk_mark; fl6.fl6_icmp_type = user_icmph.icmp6_type; fl6.fl6_icmp_code = user_icmph.icmp6_code; - security_sk_classify_flow(sk, flowi6_to_flowi(&fl6)); - if (!fl6.flowi6_oif && ipv6_addr_is_multicast(&fl6.daddr)) - fl6.flowi6_oif = np->mcast_oif; - else if (!fl6.flowi6_oif) - fl6.flowi6_oif = np->ucast_oif; - - dst = ip6_sk_dst_lookup_flow(sk, &fl6, daddr); - if (IS_ERR(dst)) - return PTR_ERR(dst); - rt = (struct rt6_info *) dst; - - np = inet6_sk(sk); - if (!np) - return -EBADF; + err = ip6_datagram_send_common(sk, msg, sin6, addr_len, &fl6, &dst, + &opt, &opt_space, &hlimit, &tclass, + &dontfrag, &connected); + if (err) + goto out; - if (!fl6.flowi6_oif && ipv6_addr_is_multicast(&fl6.daddr)) - fl6.flowi6_oif = np->mcast_oif; - else if (!fl6.flowi6_oif) - fl6.flowi6_oif = np->ucast_oif; + /* TODO: Move this check into ip6_datagram_sendmsg. */ + if (__ipv6_addr_needs_scope_id(__ipv6_addr_type(daddr)) && + !fl6.flowi6_oif) { + err = -EINVAL; + goto out; + } pfh.icmph.type = user_icmph.icmp6_type; pfh.icmph.code = user_icmph.icmp6_code; @@ -168,18 +144,10 @@ int ping_v6_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg, pfh.wcheck = 0; pfh.family = AF_INET6; - if (ipv6_addr_is_multicast(&fl6.daddr)) - hlimit = np->mcast_hops; - else - hlimit = np->hop_limit; - if (hlimit < 0) - hlimit = ip6_dst_hoplimit(dst); - + rt = (struct rt6_info *) dst; lock_sock(sk); - err = ip6_append_data(sk, ping_getfrag, &pfh, len, - 0, hlimit, - np->tclass, NULL, &fl6, rt, - MSG_DONTWAIT, np->dontfrag); + err = ip6_append_data(sk, ping_getfrag, &pfh, len, 0, hlimit, tclass, + opt, &fl6, rt, msg->msg_flags, dontfrag); if (err) { ICMP6_INC_STATS(sock_net(sk), rt->rt6i_idev, @@ -192,10 +160,9 @@ int ping_v6_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg, } release_sock(sk); - if (err) - return err; - - return len; +out: + dst_release(dst); + return err ? err : len; } #ifdef CONFIG_PROC_FS
This replaces the ad-hoc code used by ping6_sendmsg with the implementation now used by UDP, raw and L2TP sockets. This also adds the ability to set options via ancillary data, proper flowlabel validation, etc. etc. Tested: Black-box tested using user-mode Linux. - IPv6 pings using both connect()/send() and sendto() still work. - Fragmented IPv6 pings still work. - Specifying a flowlabel still works. - Attempting to send a flowlabel that is not first set via IPV6_FLOWLABEL_MGR now correctly returns EINVAL. Signed-off-by: Lorenzo Colitti <lorenzo@google.com> --- net/ipv6/ping.c | 95 +++++++++++++++++++-------------------------------------- 1 file changed, 31 insertions(+), 64 deletions(-)