| Message ID | aaa7e0af2fe535ea5216e438b5f65db88dcef8b2.1377257704.git.ffusco@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Francesco Fusco <ffusco@redhat.com> Date: Fri, 23 Aug 2013 14:19:32 +0200 > - changed the icmp_cookie ttl field from __s16 to __u8. > A value of 0 means that the TTL has not been specified Sorry, I have to ask you to change the ttl field type back to __s16 and use "-1" to mean not-specified. Zero is a valid TTL setting and it means to not allow the packet to leave this host. Please make this change and resubmit, thanks. -- 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 08/27/2013 08:56 PM, David Miller wrote: > From: Francesco Fusco <ffusco@redhat.com> > Date: Fri, 23 Aug 2013 14:19:32 +0200 > >> - changed the icmp_cookie ttl field from __s16 to __u8. >> A value of 0 means that the TTL has not been specified > > Sorry, I have to ask you to change the ttl field type back to __s16 > and use "-1" to mean not-specified. > > Zero is a valid TTL setting and it means to not allow the > packet to leave this host. Actually setsockopt() does not allow a TTL value of zero: From net/ipv4/ip_sockglue.c::do_ip_setsockopt() ----- case IP_TTL: if (optlen < 1) goto e_inval; if (val != -1 && (val < 1 || val > 255)) goto e_inval; inet->uc_ttl = val; break; --------- To make my patch consistent with the behavior of setsockopt() I also do not accept a TTL of zero in the ancillary data: + if (val < 1 || val > 255) + return -EINVAL; Therefore, if icmp_cookie->ttl has a value of 0, that could only mean that the user has not specified the TTL. I agree that could be somehow confusing to consider 0 as a non specified TTL, and that -1 would be more clear. However, it seems to me that we end up using 1 more byte in a struct that is stored on the stack for readability reasons. > Please make this change and resubmit, thanks. I can change the code as you requested despite what I wrote above, let me know. Thanks, Francesco -- 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
From: Francesco Fusco <ffusco@redhat.com> Date: Wed, 28 Aug 2013 09:56:32 +0200 > On 08/27/2013 08:56 PM, David Miller wrote: >> From: Francesco Fusco <ffusco@redhat.com> >> Date: Fri, 23 Aug 2013 14:19:32 +0200 >> >>> - changed the icmp_cookie ttl field from __s16 to __u8. >>> A value of 0 means that the TTL has not been specified >> >> Sorry, I have to ask you to change the ttl field type back to __s16 >> and use "-1" to mean not-specified. >> >> Zero is a valid TTL setting and it means to not allow the >> packet to leave this host. > > Actually setsockopt() does not allow a TTL value of zero: > > From net/ipv4/ip_sockglue.c::do_ip_setsockopt() Indeed, you are right. Please resubmit these patches for the next merge window. Thank you. -- 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
Thanks David. I will resubmit the patches as they are as soon as the merge window opens again. Best, Francesco On 09/18/2013 02:46 AM, David Miller wrote: > From: Francesco Fusco <ffusco@redhat.com> > Date: Wed, 28 Aug 2013 09:56:32 +0200 > >> On 08/27/2013 08:56 PM, David Miller wrote: >>> From: Francesco Fusco <ffusco@redhat.com> >>> Date: Fri, 23 Aug 2013 14:19:32 +0200 >>> >>>> - changed the icmp_cookie ttl field from __s16 to __u8. >>>> A value of 0 means that the TTL has not been specified >>> >>> Sorry, I have to ask you to change the ttl field type back to __s16 >>> and use "-1" to mean not-specified. >>> >>> Zero is a valid TTL setting and it means to not allow the >>> packet to leave this host. >> >> Actually setsockopt() does not allow a TTL value of zero: >> >> From net/ipv4/ip_sockglue.c::do_ip_setsockopt() > > Indeed, you are right. > > Please resubmit these patches for the next merge window. > > Thank you. > -- 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/include/net/ip.h b/include/net/ip.h index a68f838..84b5476 100644 --- a/include/net/ip.h +++ b/include/net/ip.h @@ -56,6 +56,9 @@ struct ipcm_cookie { int oif; struct ip_options_rcu *opt; __u8 tx_flags; + __u8 ttl; + __s16 tos; + char priority; }; #define IPCB(skb) ((struct inet_skb_parm*)((skb)->cb)) diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c index d9c4f11..56e3445 100644 --- a/net/ipv4/ip_sockglue.c +++ b/net/ipv4/ip_sockglue.c @@ -189,7 +189,7 @@ EXPORT_SYMBOL(ip_cmsg_recv); int ip_cmsg_send(struct net *net, struct msghdr *msg, struct ipcm_cookie *ipc) { - int err; + int err, val; struct cmsghdr *cmsg; for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) { @@ -215,6 +215,24 @@ int ip_cmsg_send(struct net *net, struct msghdr *msg, struct ipcm_cookie *ipc) ipc->addr = info->ipi_spec_dst.s_addr; break; } + case IP_TTL: + if (cmsg->cmsg_len != CMSG_LEN(sizeof(int))) + return -EINVAL; + val = *(int *)CMSG_DATA(cmsg); + if (val < 1 || val > 255) + return -EINVAL; + ipc->ttl = val; + break; + case IP_TOS: + if (cmsg->cmsg_len != CMSG_LEN(sizeof(int))) + return -EINVAL; + val = *(int *)CMSG_DATA(cmsg); + if (val < 0 || val > 255) + return -EINVAL; + ipc->tos = val; + ipc->priority = rt_tos2priority(ipc->tos); + break; + default: return -EINVAL; }
This patch enables the IP_TTL and IP_TOS values passed from userspace to be stored in the ipcm_cookie struct. Three fields are added to the struct: - the TTL, expressed as __u8. The allowed values are in the [1-255]. A value of 0 means that the TTL is not specified. - the TOS, expressed as __s16. The allowed values are in the range [0,255]. A value of -1 means that the TOS is not specified. - the priority, expressed as a char and computed when handling the ancillary data. Signed-off-by: Francesco Fusco <ffusco@redhat.com> --- v1->v2 - changed the icmp_cookie ttl field from __s16 to __u8. A value of 0 means that the TTL has not been specified - to tos field is still __s16. The user can specify values in the range 0-255 included, therefore I use a value of -1 as a flag saying that the value has not been specified - the priority it is now a char instead of a __u32, which is the return type of rt_tos2priority - improved commit message include/net/ip.h | 3 +++ net/ipv4/ip_sockglue.c | 20 +++++++++++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-)