Re: [B.A.T.M.A.N.] Antwort: Re: Antwort: Re: [PATCHv2 2/7] batman-adv: speed up dat by snooping received ip traffic

Sven Eckelmann
2016-03-02T10:23:21Z

[-- Attachment #1: Type: text/plain, Size: 2796 bytes --]

On Wednesday 02 March 2016 08:17:12 Andreas Pape wrote:
> Hello Sven,
>
> Sven Eckelmann <sven@narfation.org> schrieb am 01.03.2016 17:02:57:
>
> > You definitely have to check the size for the IP header. The vlan
> ethernet
> > header check is a different playground and mostly unrelated to your
> problem.
> >
>
> If I understood you correctly I am supposed to implement something like
> this:
>
> switch (ntohs(ethhdr->h_proto)) {
> case ETH_P_IP:
> if (unlikely(!pskb_may_pull(skb, sizeof(struct iphdr))))
> goto dropped;
> iphdr = (struct iphdr *)(skb->data + ETH_HLEN);
> /* snoop incoming traffic for dat update using the source
> mac
> * and source ip to speed up dat.
> */
> batadv_dat_entry_check(bat_priv, iphdr->saddr,
> ethhdr->h_source, vid);

I doubt that you should drop the data. You new feature is just a nice
enhancement but should not decide whether packet is valid or not. This is why
I told you about skb_header_pointer. Maybe it would also be better to move
your stuff in an extra function and avoid to add extra stuff in the softif
loop check. Maybe I will even move the softif loop check in an extra function
which would be named in a way that doesn't sound like you should add your
stuff there :)

> break;
> case ETH_P_8021Q:
> if (unlikely(!pskb_may_pull(skb, sizeof(struct
> vlan_ethhdr))))
> goto dropped;
> vhdr = (struct vlan_ethhdr *)(skb->data + ETH_HLEN);
>
> if (vhdr->h_vlan_encapsulated_proto != ethertype) {
> /* snoop incoming traffic for dat update also for
> vlan
> * tagged frames.
> */
> if (ntohs(vhdr->h_vlan_encapsulated_proto) ==
> ETH_P_IP) {
> iphdr = (struct iphdr *)(vhdr + 1);
> batadv_dat_entry_check(bat_priv,
> iphdr->saddr,
> vhdr->h_source,
> vid);

Dont forget to check the size here too.

> }
> break;
> }
> Correctly understood?
>
> Looking through the code of this function batadv_interface_rx leads me to
> another
> question: Am I right that skb->data is pointing to the beginning of the
> mac
> layer 2 header of the packet at that point in time as it isn't advanced to
> the beginning of the layer 3 header by calling eth_type_trans yet(which is
> called
> a few lines later)?

I think this should be correct.