Hello,
On Thu, 12 Apr 2018, Stephen Suryaputra wrote:
> Thanks for the feedbacks. Please see the detail below:
>
> On Wed, Apr 11, 2018 at 3:37 PM, Julian Anastasov <j...@ssi.bg> wrote:
> [snip]
> >> - __IP_INC_STATS(net, IPSTATS_MIB_INHDRERRORS);
> >> + __IP_INC_STATS(net, skb_dst(skb)->dev, IPSTATS_MIB_INHDRERRORS);
> >
> > May be skb->dev if we want to account it to the
> > input device.
> >
> Yes. I'm about to make change it but see the next one.
>
> [snip]
> >> diff --git a/net/netfilter/ipvs/ip_vs_xmit.c
> >> b/net/netfilter/ipvs/ip_vs_xmit.c
> >> index 4527921..32bd3af 100644
> >> --- a/net/netfilter/ipvs/ip_vs_xmit.c
> >> +++ b/net/netfilter/ipvs/ip_vs_xmit.c
> >> @@ -286,7 +286,7 @@ static inline bool decrement_ttl(struct netns_ipvs
> >> *ipvs,
> >> {
> >> if (ip_hdr(skb)->ttl <= 1) {
> >> /* Tell the sender its packet died... */
> >> - __IP_INC_STATS(net, IPSTATS_MIB_INHDRERRORS);
> >> + __IP_INC_STATS(net, skb_dst(skb)->dev,
> >> IPSTATS_MIB_INHDRERRORS);
> >
> > At this point, skb_dst(skb) can be:
> >
> > - input route at LOCAL_IN => dst->dev is "lo", skb->dev = input_device
> > - output route at LOCAL_OUT => dst->dev is output_device, skb->dev = NULL
> >
> > We should see this error on LOCAL_IN but better to be
> > safe: use 'skb->dev ? : skb_dst(skb)->dev' instead of just
> > 'skb_dst(skb)->dev'.
> >
> This follows v6 implementation in the same function:
>
> #ifdef CONFIG_IP_VS_IPV6
> if (skb_af == AF_INET6) {
> struct dst_entry *dst = skb_dst(skb);
>
> /* check and decrement ttl */
> if (ipv6_hdr(skb)->hop_limit <= 1) {
> /* Force OUTPUT device used as source address */
It looks like IPVS copied it from ip6_forward() but in
IPVS context it has its reason: we want ICMP to exit with
saddr=Virtual_IP. And we are at LOCAL_IN where there is no
output device like in ip6_forward(FORWARD) to use its source
address.
So, IPVS is special (both input and output path) and needs:
IPv4: skb->dev ? : skb_dst(skb)->dev
IPv6 needs fix for IPVS stats in decrement_ttl:
idev = skb->dev ? __in6_dev_get(skb->dev) : ip6_dst_idev(dst);
...
__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
Otherwise, stats will go to "lo" if ip6_dst_idev
is used for local input route.
So, for accounting on input IPv4 path skb->dev should be
used, while for IPv6 some sites may prefer to feed icmpv6_send()
with output dst->dev as device containing the source address (skb->dev).
But this is unrelated to the stats.
> skb->dev = dst->dev;
> icmpv6_send(skb, ICMPV6_TIME_EXCEED,
> ICMPV6_EXC_HOPLIMIT, 0);
> __IP6_INC_STATS(net, ip6_dst_idev(dst),
> IPSTATS_MIB_INHDRERRORS);
>
> return false;
> }
>
> /* don't propagate ttl change to cloned packets */
> if (!skb_make_writable(skb, sizeof(struct ipv6hdr)))
> return false;
>
> ipv6_hdr(skb)->hop_limit--;
> } else
> #endif
>
> [snip]
> >
> > The patch probably has other errors, for example,
> > using rt->dst.dev (lo) when rt->dst.error != 0 in ip_error,
> > may be 'dev' should be used instead...
>
> Same also here. Examples are ip6_forward and ip6_pkt_drop.
>
> I think it's better be counted in the input device for them also. Thoughts?
I think so. ipv6_rcv() works with idev = __in6_dev_get(skb->dev)
but I don't know IPv6 well and whether ip6_dst_idev(skb_dst(skb))
is correct usage for input path. It should be correct for output
path, though.
Regards
--
Julian Anastasov <j...@ssi.bg>
