| Message ID | 1411676092-16196-2-git-send-email-edumazet@google.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2014-09-25 at 13:14 -0700, Eric Dumazet wrote: > ip_options_echo() assumes struct ip_options is provided in &IPCB(skb)->opt > Lets break this assumption, but provide a helper to not change all call points. [] > diff --git a/include/net/ip.h b/include/net/ip.h [] > @@ -511,7 +513,14 @@ int ip_forward(struct sk_buff *skb); > > void ip_options_build(struct sk_buff *skb, struct ip_options *opt, > __be32 daddr, struct rtable *rt, int is_frag); > -int ip_options_echo(struct ip_options *dopt, struct sk_buff *skb); > + > +int __ip_options_echo(struct ip_options *dopt, struct sk_buff *skb, > + const struct ip_options *sopt); Thanks Eric. Unrelated: I wonder how much effort, if any, should be made to convert struct sk_buff * to const struct sk_buff * where appropriate. For instance: This __ip_options_echo could use const struct sk_buff *skb if fib_compute_spec_dst was changed to const struct sk_buff *skb. -- 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 Thu, 2014-09-25 at 13:30 -0700, Joe Perches wrote: > Unrelated: > > I wonder how much effort, if any, should be made to convert > struct sk_buff * to const struct sk_buff * where appropriate. > > For instance: > > This __ip_options_echo could use const struct sk_buff *skb > if fib_compute_spec_dst was changed to const struct sk_buff *skb. Well, this seems something certainly doable, as a follow up ;) -- 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 Thu, 2014-09-25 at 13:38 -0700, Eric Dumazet wrote: > On Thu, 2014-09-25 at 13:30 -0700, Joe Perches wrote: > > > Unrelated: > > > > I wonder how much effort, if any, should be made to convert > > struct sk_buff * to const struct sk_buff * where appropriate. > > > > For instance: > > > > This __ip_options_echo could use const struct sk_buff *skb > > if fib_compute_spec_dst was changed to const struct sk_buff *skb. > > Well, this seems something certainly doable, as a follow up ;) It's doable, but it seems a non-trivial inspection task. I believe coccinelle does not have the ability to automate this. -- 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 Thu, 25 Sep 2014, Joe Perches wrote: > On Thu, 2014-09-25 at 13:38 -0700, Eric Dumazet wrote: > > On Thu, 2014-09-25 at 13:30 -0700, Joe Perches wrote: > > > > > Unrelated: > > > > > > I wonder how much effort, if any, should be made to convert > > > struct sk_buff * to const struct sk_buff * where appropriate. > > > > > > For instance: > > > > > > This __ip_options_echo could use const struct sk_buff *skb > > > if fib_compute_spec_dst was changed to const struct sk_buff *skb. > > > > Well, this seems something certainly doable, as a follow up ;) > > It's doable, but it seems a non-trivial inspection task. > > I believe coccinelle does not have the ability to automate this. What are the exact conditions when the change is possible? I guess something like the value is only used for accessing its fields. and is not passed to any other function? Here skb is passed to a function called ip_hdr, but at least if one wanted to focus on sk_buffs then one could treat that function as a special case. julia -- 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 Thu, 25 Sep 2014, Joe Perches wrote: > On Thu, 2014-09-25 at 13:38 -0700, Eric Dumazet wrote: > > On Thu, 2014-09-25 at 13:30 -0700, Joe Perches wrote: > > > > > Unrelated: > > > > > > I wonder how much effort, if any, should be made to convert > > > struct sk_buff * to const struct sk_buff * where appropriate. > > > > > > For instance: > > > > > > This __ip_options_echo could use const struct sk_buff *skb > > > if fib_compute_spec_dst was changed to const struct sk_buff *skb. > > > > Well, this seems something certainly doable, as a follow up ;) > > It's doable, but it seems a non-trivial inspection task. > > I believe coccinelle does not have the ability to automate this. For example, can this function declare skb as const? Or would one have to know that vde_user_write does not modify the contents of skb->data as well? julia static int vde_write(int fd, struct sk_buff *skb, struct uml_net_private *lp) { struct vde_data *pri = (struct vde_data *) &lp->user; if (pri->conn != NULL) return vde_user_write((void *)pri->conn, skb->data, skb->len); printk(KERN_ERR "vde_write - we have no VDECONN to write to"); return -EBADF; } > > > -- 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, 2014-09-26 at 07:29 +0200, Julia Lawall wrote: > On Thu, 25 Sep 2014, Joe Perches wrote: > > On Thu, 2014-09-25 at 13:38 -0700, Eric Dumazet wrote: > > > On Thu, 2014-09-25 at 13:30 -0700, Joe Perches wrote: [] > > > > I wonder how much effort, if any, should be made to convert > > > > struct sk_buff * to const struct sk_buff * where appropriate. > > > > > > > > For instance:n > > > > > > > > This __ip_options_echo could use const struct sk_buff *skb > > > > if fib_compute_spec_dst was changed to const struct sk_buff *skb. > > > > > > Well, this seems something certainly doable, as a follow up ;) > > > > It's doable, but it seems a non-trivial inspection task. > > > > I believe coccinelle does not have the ability to automate this. > > What are the exact conditions when the change is possible? > > I guess something like the value is only used for accessing its fields. > and is not passed to any other function? I expect the entire call tree needs to be known and inspected for modification of fields. -- 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 fcd9068fb8c3..0bb620702929 100644 --- a/include/net/ip.h +++ b/include/net/ip.h @@ -180,8 +180,10 @@ static inline __u8 ip_reply_arg_flowi_flags(const struct ip_reply_arg *arg) return (arg->flags & IP_REPLY_ARG_NOSRCCHECK) ? FLOWI_FLAG_ANYSRC : 0; } -void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr, - __be32 saddr, const struct ip_reply_arg *arg, +void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, + const struct ip_options *sopt, + __be32 daddr, __be32 saddr, + const struct ip_reply_arg *arg, unsigned int len); #define IP_INC_STATS(net, field) SNMP_INC_STATS64((net)->mib.ip_statistics, field) @@ -511,7 +513,14 @@ int ip_forward(struct sk_buff *skb); void ip_options_build(struct sk_buff *skb, struct ip_options *opt, __be32 daddr, struct rtable *rt, int is_frag); -int ip_options_echo(struct ip_options *dopt, struct sk_buff *skb); + +int __ip_options_echo(struct ip_options *dopt, struct sk_buff *skb, + const struct ip_options *sopt); +static inline int ip_options_echo(struct ip_options *dopt, struct sk_buff *skb) +{ + return __ip_options_echo(dopt, skb, &IPCB(skb)->opt); +} + void ip_options_fragment(struct sk_buff *skb); int ip_options_compile(struct net *net, struct ip_options *opt, struct sk_buff *skb); diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c index ad382499bace..5b3d91be2db0 100644 --- a/net/ipv4/ip_options.c +++ b/net/ipv4/ip_options.c @@ -87,17 +87,15 @@ void ip_options_build(struct sk_buff *skb, struct ip_options *opt, * NOTE: dopt cannot point to skb. */ -int ip_options_echo(struct ip_options *dopt, struct sk_buff *skb) +int __ip_options_echo(struct ip_options *dopt, struct sk_buff *skb, + const struct ip_options *sopt) { - const struct ip_options *sopt; unsigned char *sptr, *dptr; int soffset, doffset; int optlen; memset(dopt, 0, sizeof(struct ip_options)); - sopt = &(IPCB(skb)->opt); - if (sopt->optlen == 0) return 0; diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c index 215af2b155cb..c8fa62476461 100644 --- a/net/ipv4/ip_output.c +++ b/net/ipv4/ip_output.c @@ -1522,8 +1522,10 @@ static DEFINE_PER_CPU(struct inet_sock, unicast_sock) = { .uc_ttl = -1, }; -void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr, - __be32 saddr, const struct ip_reply_arg *arg, +void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, + const struct ip_options *sopt, + __be32 daddr, __be32 saddr, + const struct ip_reply_arg *arg, unsigned int len) { struct ip_options_data replyopts; @@ -1534,7 +1536,7 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr, struct sock *sk; struct inet_sock *inet; - if (ip_options_echo(&replyopts.opt.opt, skb)) + if (__ip_options_echo(&replyopts.opt.opt, skb, sopt)) return; ipc.addr = daddr; diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index 3b2e49cb2b61..28ab90382c01 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -681,8 +681,9 @@ static void tcp_v4_send_reset(struct sock *sk, struct sk_buff *skb) net = dev_net(skb_dst(skb)->dev); arg.tos = ip_hdr(skb)->tos; - ip_send_unicast_reply(net, skb, ip_hdr(skb)->saddr, - ip_hdr(skb)->daddr, &arg, arg.iov[0].iov_len); + ip_send_unicast_reply(net, skb, &TCP_SKB_CB(skb)->header.h4.opt, + ip_hdr(skb)->saddr, ip_hdr(skb)->daddr, + &arg, arg.iov[0].iov_len); TCP_INC_STATS_BH(net, TCP_MIB_OUTSEGS); TCP_INC_STATS_BH(net, TCP_MIB_OUTRSTS); @@ -764,8 +765,9 @@ static void tcp_v4_send_ack(struct sk_buff *skb, u32 seq, u32 ack, if (oif) arg.bound_dev_if = oif; arg.tos = tos; - ip_send_unicast_reply(net, skb, ip_hdr(skb)->saddr, - ip_hdr(skb)->daddr, &arg, arg.iov[0].iov_len); + ip_send_unicast_reply(net, skb, &TCP_SKB_CB(skb)->header.h4.opt, + ip_hdr(skb)->saddr, ip_hdr(skb)->daddr, + &arg, arg.iov[0].iov_len); TCP_INC_STATS_BH(net, TCP_MIB_OUTSEGS); }
ip_options_echo() assumes struct ip_options is provided in &IPCB(skb)->opt Lets break this assumption, but provide a helper to not change all call points. ip_send_unicast_reply() gets a new struct ip_options pointer. Signed-off-by: Eric Dumazet <edumazet@google.com> --- include/net/ip.h | 15 ++++++++++++--- net/ipv4/ip_options.c | 6 ++---- net/ipv4/ip_output.c | 8 +++++--- net/ipv4/tcp_ipv4.c | 10 ++++++---- 4 files changed, 25 insertions(+), 14 deletions(-)