[RFC,4/6] batman-adv: Adds necessary functions for NDP, like checking if a packet is valid or creating a Neighbor Advertisement

From: Mihail Costea <mihail.costea90@gmail.com>

Adds functions needed for NDP snooping, like getting the IPv6 addresses
or getting the target HW address from an Neighbor Advertisement (NA).
Also added functions to create NA for Neighbor Solicitations
that have already the HW address in DAT.

Signed-off-by: Mihail Costea <mihail.costea90@gmail.com>
Signed-off-by: Stefan Popa <Stefan.A.Popa@intel.com>
Reviewed-by: Stefan Popa <Stefan.A.Popa@intel.com>

---
 distributed-arp-table.c |  319 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 319 insertions(+)

On Mon, Jul 08, 2013 at 03:12:43AM +0300, mihail.costea2005@gmail.com wrote:
> From: Mihail Costea <mihail.costea90@gmail.com>
> 
> Adds functions needed for NDP snooping, like getting the IPv6 addresses
> or getting the target HW address from an Neighbor Advertisement (NA).

typo: an -> a

> Also added functions to create NA for Neighbor Solicitations

use always the same form: added -> adds

> that have already the HW address in DAT.
> 
> Signed-off-by: Mihail Costea <mihail.costea90@gmail.com>
> Signed-off-by: Stefan Popa <Stefan.A.Popa@intel.com>
> Reviewed-by: Stefan Popa <Stefan.A.Popa@intel.com>
> 
> ---
>  distributed-arp-table.c |  319 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 319 insertions(+)
> 
> diff --git a/distributed-arp-table.c b/distributed-arp-table.c
> index f941913..d0b9e95 100644
> --- a/distributed-arp-table.c
> +++ b/distributed-arp-table.c
> @@ -20,7 +20,9 @@
>  #include <linux/if_ether.h>
>  #include <linux/if_arp.h>
>  #include <linux/if_vlan.h>
> +#include <net/addrconf.h>
>  #include <net/arp.h>
> +#include <net/ipv6.h>
>  
>  #include "main.h"
>  #include "hash.h"
> @@ -981,6 +983,323 @@ static unsigned short batadv_dat_get_vid(struct sk_buff *skb, int *hdr_size)
>  	return vid;
>  }
>  
> +#if IS_ENABLED(CONFIG_IPV6)
> +/**
> + * batadv_ndisc_hw_src - get source hw address from a packet
> + * @skb: packet to check
> + * @hdr_size: size of the encapsulation header
> + *
> + * Returns source hw address of the skb packet.
> + */
> +static uint8_t *batadv_ndisc_hw_src(struct sk_buff *skb, int hdr_size)
> +{
> +	struct ethhdr *ethhdr;

please put a blank line after variable declarations.

> +	ethhdr = (struct ethhdr *)(skb->data + hdr_size);
> +	return (uint8_t *)ethhdr->h_source;
> +}
> +
> +/**
> + * batadv_ndisc_hw_dst - get destination hw address from a packet
> + * @skb: packet to check
> + * @hdr_size: size of the encapsulation header
> + *
> + * Returns destination hw address of the skb packet.
> + */
> +static uint8_t *batadv_ndisc_hw_dst(struct sk_buff *skb, int hdr_size)
> +{
> +	struct ethhdr *ethhdr;

same here

> +	ethhdr = (struct ethhdr *)(skb->data + hdr_size);
> +	return (uint8_t *)ethhdr->h_dest;
> +}
> +
> +/**
> + * batadv_ndisc_ipv6_src - get source IPv6 address from a packet
> + * @skb: packet to check
> + * @hdr_size: size of the encapsulation header
> + *
> + * Returns source IPv6 address of the skb packet.
> + */
> +static struct in6_addr *batadv_ndisc_ipv6_src(struct sk_buff *skb,
> +					      int hdr_size)
> +{
> +	struct ipv6hdr *ipv6hdr;

same here

> +	ipv6hdr = (struct ipv6hdr *)(skb->data + hdr_size + ETH_HLEN);
> +	return &ipv6hdr->saddr;
> +}
> +
> +/**
> + * batadv_ndisc_ipv6_dst - get destination IPv6 address from a packet
> + * @skb: packet to check
> + * @hdr_size: size of the encapsulation header
> + *
> + * Returns destination IPv6 address of the skb packet.
> + */
> +static struct in6_addr *batadv_ndisc_ipv6_dst(struct sk_buff *skb,
> +					      int hdr_size)
> +{
> +	struct ipv6hdr *ipv6hdr;

same here

> +	ipv6hdr = (struct ipv6hdr *)(skb->data + hdr_size + ETH_HLEN);
> +	return &ipv6hdr->daddr;
> +}
> +
> +/**
> + * batadv_ndisc_ipv6_target - get target IPv6 address from a NS/NA packet
> + * @skb: packet to check
> + * @hdr_size: size of the encapsulation header
> + *
> + * Returns target IPv6 address of the skb packet.
> + */
> +static struct in6_addr *batadv_ndisc_ipv6_target(struct sk_buff *skb,
> +						 int hdr_size)
> +{
> +	return (struct in6_addr *)(skb->data + hdr_size + ETH_HLEN +
> +			sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr));

please, use the needed local variables to make this statement more readable and
and to align it in a proper way.

> +}
> +
> +/**
> + * batadv_ndisc_hw_opt - get hw address from NS/NA packet options
> + * @skb: packet to check
> + * @hdr_size: size of the encapsulation header
> + * @type: type of the address (ND_OPT_SOURCE_LL_ADDR or ND_OPT_TARGET_LL_ADDR)
> + *
> + * The address can be either the source link-layer address
> + * or the target link-layer address.
> + * For more info see RFC2461.
> + *
> + * Returns hw address from packet options.
> + */
> +static uint8_t *batadv_ndisc_hw_opt(struct sk_buff *skb, int hdr_size,
> +				    uint8_t type)
> +{
> +	unsigned char *opt_addr;
> +
> +	opt_addr = skb->data + hdr_size + ETH_HLEN +
> +			sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr) +
> +			sizeof(struct in6_addr);
> +

align this statement properly. it should be:

like_this = this + that +
	    and_whatever +
	    we_need;

> +	/* test if option header is ok */

Please, be a bit more verbose in this comment. What are we checking here? 
why != 1? What does that mean? Otherwise it will be difficult for other people
out of these patches business to understand

> +	if (*opt_addr != type || *(opt_addr + 1) != 1)
> +		return NULL;
> +	return (uint8_t *)(opt_addr + 2);

and why skipping the initial two bytes? Maybe you should describe what opt_addr
contains? this will probably help to better understand what this statement is
doing.

> +}
> +
> +/**
> + * batadv_ndisc_get_type - get icmp6 packet type

I think you can directly call this function "batadv_icmpv6_get_type".
It may be re-used in the future for the same purpose but somewhere else :)

> + * @bat_priv: the bat priv with all the soft interface information
> + * @skb: packet to check
> + * @hdr_size: size of the encapsulation header
> + *
> + * Returns the icmp6 type, or 0 if packet is not a valid icmp6 packet.
> + */
> +static __u8 batadv_ndisc_get_type(struct batadv_priv *bat_priv,

in the batman-adv code we use stdint: __u8 -> uint8_t (there are other spots
where you use __8)

> +				  struct sk_buff *skb, int hdr_size)
> +{
> +	struct ethhdr *ethhdr;
> +	struct ipv6hdr *ipv6hdr;
> +	struct icmp6hdr *icmp6hdr;
> +	__u8 type = 0;
> +
> +	/* pull headers */
> +	if (unlikely(!pskb_may_pull(skb, hdr_size + ETH_HLEN +
> +			sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr))))

please properly align this statement too. I think checkpatch would have
complained about this. You can use tabs + spaces to correctly align.

> +		goto out;
> +
> +	/* get the ethernet header */

remove this comment :) Variables are named properly and this is obvious.

> +	ethhdr = (struct ethhdr *)(skb->data + hdr_size);
> +	if (ethhdr->h_proto != htons(ETH_P_IPV6))
> +		goto out;
> +
> +	/* get the ipv6 header */

same

> +	ipv6hdr = (struct ipv6hdr *)(skb->data + hdr_size + ETH_HLEN);
> +	if (ipv6hdr->nexthdr != IPPROTO_ICMPV6)
> +		goto out;
> +
> +	/* get the icmpv6 header */
> +	icmp6hdr = (struct icmp6hdr *)(skb->data + hdr_size + ETH_HLEN +
> +			sizeof(struct ipv6hdr));

alignment..should line up to the opening parenthesis.

(blablabla *)(like
	      this);

> +
> +	/* check whether the ndisc packet carries a valid icmp6 header */
> +	if (ipv6hdr->hop_limit != 255)
> +		goto out;
> +
> +	if (icmp6hdr->icmp6_code != 0)
> +		goto out;
> +
> +	type = icmp6hdr->icmp6_type;
> +out:
> +	return type;
> +}
> +
> +/**
> + * batadv_ndisc_is_valid - check if a ndisc packet is valid
> + * @bat_priv: the bat priv with all the soft interface information
> + * @skb: packet to check
> + * @hdr_size: size of the encapsulation header
> + * @ndisc_type: type of ndisc packet to check
> + *
> + * Check if all IPs are valid (source, destination, target) and if
> + * options hw address is valid too.
> + * Some options might be ignored, like NS packets sent automatically
> + * for verification of the reachability of a neighbor.
> + *
> + * Returns true if packet is valid, otherwise false if invalid or ignored.
> + */
> +static bool batadv_ndisc_is_valid(struct batadv_priv *bat_priv,
> +				  struct sk_buff *skb, int hdr_size,
> +				  int ndisc_type)
> +{
> +	uint8_t *hw_target = NULL;
> +	struct in6_addr *ipv6_src, *ipv6_dst, *ipv6_target;
> +	__u8 type = batadv_ndisc_get_type(bat_priv, skb, hdr_size);
> +	int ndisc_hdr_len = hdr_size + ETH_HLEN + sizeof(struct ipv6hdr) +
> +			    sizeof(struct icmp6hdr) + sizeof(struct in6_addr) +
> +			    8; /* ndisc options length */

when the assignment is too long, please do it after the declarations. Improves
readability.


> +
> +	if (type != ndisc_type)
> +		return false;
> +	if (unlikely(!pskb_may_pull(skb, ndisc_hdr_len)))
> +		return false;
> +
> +	/* Check for bad NA/NS. If the ndisc message is not sane, DAT
> +	 * will simply ignore it
> +	 */
> +	if (type == NDISC_NEIGHBOUR_SOLICITATION)
> +		hw_target = batadv_ndisc_hw_opt(skb, hdr_size,
> +						ND_OPT_SOURCE_LL_ADDR);
> +	else if (type == NDISC_NEIGHBOUR_ADVERTISEMENT)
> +		hw_target = batadv_ndisc_hw_opt(skb, hdr_size,
> +						ND_OPT_TARGET_LL_ADDR);
> +
> +	if (!hw_target || is_zero_ether_addr(hw_target) ||
> +	    is_multicast_ether_addr(hw_target))
> +		return false;
> +
> +	ipv6_src = batadv_ndisc_ipv6_src(skb, hdr_size);
> +	ipv6_dst = batadv_ndisc_ipv6_dst(skb, hdr_size);
> +	ipv6_target = batadv_ndisc_ipv6_target(skb, hdr_size);
> +	if (ipv6_addr_loopback(ipv6_src) || ipv6_addr_loopback(ipv6_dst) ||
> +	    ipv6_addr_is_multicast(ipv6_src) ||
> +	    ipv6_addr_is_multicast(ipv6_target))
> +		return false;
> +
> +	/* ignore messages with unspecified address */
> +	if (ipv6_addr_any(ipv6_src) || ipv6_addr_any(ipv6_dst) ||
> +	    ipv6_addr_any(ipv6_target))
> +		return false;
> +
> +	/* ignore the verification of the reachability of a neighbor */
> +	if (type == NDISC_NEIGHBOUR_SOLICITATION &&
> +	    !ipv6_addr_is_multicast(ipv6_dst))
> +		return false;
> +
> +	return true;
> +}
> +
> +static void batadv_ndisc_ip6_hdr(struct sock *sk, struct sk_buff *skb,
> +				 struct net_device *dev,
> +				 const struct in6_addr *ipv6_src,
> +				 const struct in6_addr *ipv6_dst,
> +				 int proto, int len)

alignment?

> +{
> +	struct ipv6_pinfo *np = inet6_sk(sk);
> +	struct ipv6hdr *hdr;
> +
> +	skb->protocol = htons(ETH_P_IPV6);
> +	skb->dev = dev;
> +
> +	skb_reset_network_header(skb);
> +	skb_put(skb, sizeof(struct ipv6hdr));
> +	hdr = ipv6_hdr(skb);
> +
> +	*(__be32 *)hdr = htonl(0x60000000);

What does this constant mean? you are writing on the IPv6 header?
why not writing in one of its fields (I guess you wanted to write in the first
one)?

> +
> +	hdr->payload_len = htons(len);

I think len can be declared int16_t to avoid using wrong values?
(here and where you call this function)

> +	hdr->nexthdr = proto;
> +	hdr->hop_limit = np->hop_limit;
> +
> +	hdr->saddr = *ipv6_src;
> +	hdr->daddr = *ipv6_dst;
> +}
> +
> +/**
> + * batadv_ndisc_create_na - create an NA for a solicited NS
> + * @net_device: the devices for which the packet is created
> + * @ipv6_dst: destination IPv6
> + * @ipv6_target: target IPv6 (same with source IPv6)
> + * @hw_dst: destination HW Address
> + * @hw_target: target HW Address (same with source HW Address)
> + * @router: 1 if target is a router, else 0
> + * @solicited: 1 if this is a solicited NA, else 0
> + * @override: 1 if the target entry should be override, else 0
> + *
> + * For more info see RFC2461.

If you want to cite the RFC think you should provide also a section?
The "Neighbor Discovery for IP Version 6" RFC is a bit too much is I only want
to understand what a NA or NS is and how the NA is created.

By the way, ok for reading the RFC, but I think you can write a bit more about
what the function is doing: e.g. create a new skb containing an NA which fields
are initialised with the value passed as argument. Seems obvious, but better
safe than sorry :) Kernel Doc is shown without the code if you compile it.

> + *
> + * Returns the newly created skb, otherwise NULL.
> + */
> +static struct
> +sk_buff *batadv_ndisc_create_na(struct net_device *dev,
> +				const struct in6_addr *ipv6_dst,
> +				const struct in6_addr *ipv6_target,
> +				const uint8_t *hw_dst,
> +				const uint8_t *hw_target,
> +				int router, int solicited, int override)

if these variables can only be 0 or 1, why not using bool?

> +{
> +	struct net *net = dev_net(dev);
> +	struct sock *sk = net->ipv6.ndisc_sk;
> +	struct sk_buff *skb;
> +	struct icmp6hdr *icmp6hdr;
> +	int hlen = LL_RESERVED_SPACE(dev);
> +	int tlen = dev->needed_tailroom;
> +	int len, err;
> +	u8 *opt;

uint8_t

> +
> +	/* alloc space for skb */
> +	len = sizeof(struct icmp6hdr) + sizeof(*ipv6_target) + 8;

write in the comment what is this 8. Otherwise, since you use it more than once,
create a define with a descriptive name and it instead of the 8.

> +	skb = sock_alloc_send_skb(sk,
> +				  (MAX_HEADER + sizeof(struct ipv6hdr) +

					why these parenthesis here?

> +				   len + hlen + tlen),

I guess hlen is ETH_HLEN? what does LL_RESERVED_SPACE exactly return?
and why using needed_tailroom? what does it comport in this case?
We never used that during SKB allocation...this is why I am asking.

> +				  1, &err);

and why are you using this function to allocate the skb? In the rest of the code
we always use netdev_alloc_skb_ip_align() (that also takes care of properly
aligning the skb).

> +	if (!skb)
> +		return NULL;
> +
> +	skb_reserve(skb, hlen);
> +	batadv_ndisc_ip6_hdr(sk, skb, dev, ipv6_target, ipv6_dst,
> +			     IPPROTO_ICMPV6, len);
> +
> +	skb->transport_header = skb->tail;

why you care about setting the transport header?
You could also use skb_set_transport_header() passing a proper offset rather
than directly using skb->tail. Following the skb logic is "sometimes" not
straightforward, therefore it is better to use the provided functions when
possible.

> +	skb_put(skb, len);
> +
> +	/* fill the device header for the NA frame */
> +	if (dev_hard_header(skb, dev, ETH_P_IPV6, hw_dst,
> +			    hw_target, skb->len) < 0) {
> +		kfree_skb(skb);
> +		return NULL;
> +	}

mh..we never used this function as we assume that the interface below batman-adv
is carrying 802.3 frame only. But looks interesting :)

> +
> +	/* set icmpv6 header */
> +	icmp6hdr = (struct icmp6hdr *)skb_transport_header(skb);

ah now I understood why you have set the transport_header :)

> +	icmp6hdr->icmp6_type = NDISC_NEIGHBOUR_ADVERTISEMENT;
> +	icmp6hdr->icmp6_router = router;
> +	icmp6hdr->icmp6_solicited = solicited;
> +	icmp6hdr->icmp6_override = override;
> +
> +	/* set NA target and options */
> +	opt = skb_transport_header(skb) + sizeof(*icmp6hdr);
> +	*(struct in6_addr *)opt = *ipv6_target;
> +	opt += sizeof(*ipv6_target);
> +
> +	opt[0] = ND_OPT_TARGET_LL_ADDR;
> +	opt[1] = 1;
> +	memcpy(opt + 2, hw_target, dev->addr_len);

ah, these are the famous 8 bytes :) So hard to discover! :D

> +
> +	icmp6hdr->icmp6_cksum = csum_ipv6_magic(ipv6_target, ipv6_dst, len,
> +						IPPROTO_ICMPV6,
> +						csum_partial(icmp6hdr, len, 0));
> +
> +	return skb;
> +}
> +#endif
> +
>  /**
>   * batadv_dat_snoop_outgoing_pkt_request - snoop the ARP request and try to
>   * answer using DAT
> -- 
> 1.7.10.4

Hi,

On 10 August 2013 05:20, Antonio Quartulli <ordex@autistici.org> wrote:
> On Mon, Jul 08, 2013 at 03:12:43AM +0300, mihail.costea2005@gmail.com wrote:
>> From: Mihail Costea <mihail.costea90@gmail.com>
>>
>> Adds functions needed for NDP snooping, like getting the IPv6 addresses
>> or getting the target HW address from an Neighbor Advertisement (NA).
>
> typo: an -> a
>
>> Also added functions to create NA for Neighbor Solicitations
>
> use always the same form: added -> adds
>
>> that have already the HW address in DAT.
>>
>> Signed-off-by: Mihail Costea <mihail.costea90@gmail.com>
>> Signed-off-by: Stefan Popa <Stefan.A.Popa@intel.com>
>> Reviewed-by: Stefan Popa <Stefan.A.Popa@intel.com>
>>
>> ---
>>  distributed-arp-table.c |  319 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 319 insertions(+)
>>
>> diff --git a/distributed-arp-table.c b/distributed-arp-table.c
>> index f941913..d0b9e95 100644
>> --- a/distributed-arp-table.c
>> +++ b/distributed-arp-table.c
>> @@ -20,7 +20,9 @@
>>  #include <linux/if_ether.h>
>>  #include <linux/if_arp.h>
>>  #include <linux/if_vlan.h>
>> +#include <net/addrconf.h>
>>  #include <net/arp.h>
>> +#include <net/ipv6.h>
>>
>>  #include "main.h"
>>  #include "hash.h"
>> @@ -981,6 +983,323 @@ static unsigned short batadv_dat_get_vid(struct sk_buff *skb, int *hdr_size)
>>       return vid;
>>  }
>>
>> +#if IS_ENABLED(CONFIG_IPV6)
>> +/**
>> + * batadv_ndisc_hw_src - get source hw address from a packet
>> + * @skb: packet to check
>> + * @hdr_size: size of the encapsulation header
>> + *
>> + * Returns source hw address of the skb packet.
>> + */
>> +static uint8_t *batadv_ndisc_hw_src(struct sk_buff *skb, int hdr_size)
>> +{
>> +     struct ethhdr *ethhdr;
>
> please put a blank line after variable declarations.
>
>> +     ethhdr = (struct ethhdr *)(skb->data + hdr_size);
>> +     return (uint8_t *)ethhdr->h_source;
>> +}
>> +
>> +/**
>> + * batadv_ndisc_hw_dst - get destination hw address from a packet
>> + * @skb: packet to check
>> + * @hdr_size: size of the encapsulation header
>> + *
>> + * Returns destination hw address of the skb packet.
>> + */
>> +static uint8_t *batadv_ndisc_hw_dst(struct sk_buff *skb, int hdr_size)
>> +{
>> +     struct ethhdr *ethhdr;
>
> same here
>
>> +     ethhdr = (struct ethhdr *)(skb->data + hdr_size);
>> +     return (uint8_t *)ethhdr->h_dest;
>> +}
>> +
>> +/**
>> + * batadv_ndisc_ipv6_src - get source IPv6 address from a packet
>> + * @skb: packet to check
>> + * @hdr_size: size of the encapsulation header
>> + *
>> + * Returns source IPv6 address of the skb packet.
>> + */
>> +static struct in6_addr *batadv_ndisc_ipv6_src(struct sk_buff *skb,
>> +                                           int hdr_size)
>> +{
>> +     struct ipv6hdr *ipv6hdr;
>
> same here
>
>> +     ipv6hdr = (struct ipv6hdr *)(skb->data + hdr_size + ETH_HLEN);
>> +     return &ipv6hdr->saddr;
>> +}
>> +
>> +/**
>> + * batadv_ndisc_ipv6_dst - get destination IPv6 address from a packet
>> + * @skb: packet to check
>> + * @hdr_size: size of the encapsulation header
>> + *
>> + * Returns destination IPv6 address of the skb packet.
>> + */
>> +static struct in6_addr *batadv_ndisc_ipv6_dst(struct sk_buff *skb,
>> +                                           int hdr_size)
>> +{
>> +     struct ipv6hdr *ipv6hdr;
>
> same here
>
>> +     ipv6hdr = (struct ipv6hdr *)(skb->data + hdr_size + ETH_HLEN);
>> +     return &ipv6hdr->daddr;
>> +}
>> +
>> +/**
>> + * batadv_ndisc_ipv6_target - get target IPv6 address from a NS/NA packet
>> + * @skb: packet to check
>> + * @hdr_size: size of the encapsulation header
>> + *
>> + * Returns target IPv6 address of the skb packet.
>> + */
>> +static struct in6_addr *batadv_ndisc_ipv6_target(struct sk_buff *skb,
>> +                                              int hdr_size)
>> +{
>> +     return (struct in6_addr *)(skb->data + hdr_size + ETH_HLEN +
>> +                     sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr));
>
> please, use the needed local variables to make this statement more readable and
> and to align it in a proper way.
>
>> +}
>> +
>> +/**
>> + * batadv_ndisc_hw_opt - get hw address from NS/NA packet options
>> + * @skb: packet to check
>> + * @hdr_size: size of the encapsulation header
>> + * @type: type of the address (ND_OPT_SOURCE_LL_ADDR or ND_OPT_TARGET_LL_ADDR)
>> + *
>> + * The address can be either the source link-layer address
>> + * or the target link-layer address.
>> + * For more info see RFC2461.
>> + *
>> + * Returns hw address from packet options.
>> + */
>> +static uint8_t *batadv_ndisc_hw_opt(struct sk_buff *skb, int hdr_size,
>> +                                 uint8_t type)
>> +{
>> +     unsigned char *opt_addr;
>> +
>> +     opt_addr = skb->data + hdr_size + ETH_HLEN +
>> +                     sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr) +
>> +                     sizeof(struct in6_addr);
>> +
>
> align this statement properly. it should be:
>
> like_this = this + that +
>             and_whatever +
>             we_need;
>
>> +     /* test if option header is ok */
>
> Please, be a bit more verbose in this comment. What are we checking here?
> why != 1? What does that mean? Otherwise it will be difficult for other people
> out of these patches business to understand
>
>> +     if (*opt_addr != type || *(opt_addr + 1) != 1)
>> +             return NULL;
>> +     return (uint8_t *)(opt_addr + 2);
>
> and why skipping the initial two bytes? Maybe you should describe what opt_addr
> contains? this will probably help to better understand what this statement is
> doing.
>
>> +}
>> +
>> +/**
>> + * batadv_ndisc_get_type - get icmp6 packet type
>
> I think you can directly call this function "batadv_icmpv6_get_type".
> It may be re-used in the future for the same purpose but somewhere else :)
>
>> + * @bat_priv: the bat priv with all the soft interface information
>> + * @skb: packet to check
>> + * @hdr_size: size of the encapsulation header
>> + *
>> + * Returns the icmp6 type, or 0 if packet is not a valid icmp6 packet.
>> + */
>> +static __u8 batadv_ndisc_get_type(struct batadv_priv *bat_priv,
>
> in the batman-adv code we use stdint: __u8 -> uint8_t (there are other spots
> where you use __8)
>
>> +                               struct sk_buff *skb, int hdr_size)
>> +{
>> +     struct ethhdr *ethhdr;
>> +     struct ipv6hdr *ipv6hdr;
>> +     struct icmp6hdr *icmp6hdr;
>> +     __u8 type = 0;
>> +
>> +     /* pull headers */
>> +     if (unlikely(!pskb_may_pull(skb, hdr_size + ETH_HLEN +
>> +                     sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr))))
>
> please properly align this statement too. I think checkpatch would have
> complained about this. You can use tabs + spaces to correctly align.
>

Some of the code style problems were added by git mail it seems. In my
code the alignment is correct (I did you --strict).
But I didn't new comments should be :).
/**
 *
 */

>> +             goto out;
>> +
>> +     /* get the ethernet header */
>
> remove this comment :) Variables are named properly and this is obvious.
>
>> +     ethhdr = (struct ethhdr *)(skb->data + hdr_size);
>> +     if (ethhdr->h_proto != htons(ETH_P_IPV6))
>> +             goto out;
>> +
>> +     /* get the ipv6 header */
>
> same
>
>> +     ipv6hdr = (struct ipv6hdr *)(skb->data + hdr_size + ETH_HLEN);
>> +     if (ipv6hdr->nexthdr != IPPROTO_ICMPV6)
>> +             goto out;
>> +
>> +     /* get the icmpv6 header */
>> +     icmp6hdr = (struct icmp6hdr *)(skb->data + hdr_size + ETH_HLEN +
>> +                     sizeof(struct ipv6hdr));
>
> alignment..should line up to the opening parenthesis.
>
> (blablabla *)(like
>               this);
>
>> +
>> +     /* check whether the ndisc packet carries a valid icmp6 header */
>> +     if (ipv6hdr->hop_limit != 255)
>> +             goto out;
>> +
>> +     if (icmp6hdr->icmp6_code != 0)
>> +             goto out;
>> +
>> +     type = icmp6hdr->icmp6_type;
>> +out:
>> +     return type;
>> +}
>> +
>> +/**
>> + * batadv_ndisc_is_valid - check if a ndisc packet is valid
>> + * @bat_priv: the bat priv with all the soft interface information
>> + * @skb: packet to check
>> + * @hdr_size: size of the encapsulation header
>> + * @ndisc_type: type of ndisc packet to check
>> + *
>> + * Check if all IPs are valid (source, destination, target) and if
>> + * options hw address is valid too.
>> + * Some options might be ignored, like NS packets sent automatically
>> + * for verification of the reachability of a neighbor.
>> + *
>> + * Returns true if packet is valid, otherwise false if invalid or ignored.
>> + */
>> +static bool batadv_ndisc_is_valid(struct batadv_priv *bat_priv,
>> +                               struct sk_buff *skb, int hdr_size,
>> +                               int ndisc_type)
>> +{
>> +     uint8_t *hw_target = NULL;
>> +     struct in6_addr *ipv6_src, *ipv6_dst, *ipv6_target;
>> +     __u8 type = batadv_ndisc_get_type(bat_priv, skb, hdr_size);
>> +     int ndisc_hdr_len = hdr_size + ETH_HLEN + sizeof(struct ipv6hdr) +
>> +                         sizeof(struct icmp6hdr) + sizeof(struct in6_addr) +
>> +                         8; /* ndisc options length */
>
> when the assignment is too long, please do it after the declarations. Improves
> readability.
>
>
>> +
>> +     if (type != ndisc_type)
>> +             return false;
>> +     if (unlikely(!pskb_may_pull(skb, ndisc_hdr_len)))
>> +             return false;
>> +
>> +     /* Check for bad NA/NS. If the ndisc message is not sane, DAT
>> +      * will simply ignore it
>> +      */
>> +     if (type == NDISC_NEIGHBOUR_SOLICITATION)
>> +             hw_target = batadv_ndisc_hw_opt(skb, hdr_size,
>> +                                             ND_OPT_SOURCE_LL_ADDR);
>> +     else if (type == NDISC_NEIGHBOUR_ADVERTISEMENT)
>> +             hw_target = batadv_ndisc_hw_opt(skb, hdr_size,
>> +                                             ND_OPT_TARGET_LL_ADDR);
>> +
>> +     if (!hw_target || is_zero_ether_addr(hw_target) ||
>> +         is_multicast_ether_addr(hw_target))
>> +             return false;
>> +
>> +     ipv6_src = batadv_ndisc_ipv6_src(skb, hdr_size);
>> +     ipv6_dst = batadv_ndisc_ipv6_dst(skb, hdr_size);
>> +     ipv6_target = batadv_ndisc_ipv6_target(skb, hdr_size);
>> +     if (ipv6_addr_loopback(ipv6_src) || ipv6_addr_loopback(ipv6_dst) ||
>> +         ipv6_addr_is_multicast(ipv6_src) ||
>> +         ipv6_addr_is_multicast(ipv6_target))
>> +             return false;
>> +
>> +     /* ignore messages with unspecified address */
>> +     if (ipv6_addr_any(ipv6_src) || ipv6_addr_any(ipv6_dst) ||
>> +         ipv6_addr_any(ipv6_target))
>> +             return false;
>> +
>> +     /* ignore the verification of the reachability of a neighbor */
>> +     if (type == NDISC_NEIGHBOUR_SOLICITATION &&
>> +         !ipv6_addr_is_multicast(ipv6_dst))
>> +             return false;
>> +
>> +     return true;
>> +}
>> +
>> +static void batadv_ndisc_ip6_hdr(struct sock *sk, struct sk_buff *skb,
>> +                              struct net_device *dev,
>> +                              const struct in6_addr *ipv6_src,
>> +                              const struct in6_addr *ipv6_dst,
>> +                              int proto, int len)
>
> alignment?
>
>> +{
>> +     struct ipv6_pinfo *np = inet6_sk(sk);
>> +     struct ipv6hdr *hdr;
>> +
>> +     skb->protocol = htons(ETH_P_IPV6);
>> +     skb->dev = dev;
>> +
>> +     skb_reset_network_header(skb);
>> +     skb_put(skb, sizeof(struct ipv6hdr));
>> +     hdr = ipv6_hdr(skb);
>> +
>> +     *(__be32 *)hdr = htonl(0x60000000);
>
> What does this constant mean? you are writing on the IPv6 header?
> why not writing in one of its fields (I guess you wanted to write in the first
> one)?
>

I mostly took this function from another source file as it is, but it
was static so I couldn't use it directly.
I will make the changes.

>> +
>> +     hdr->payload_len = htons(len);
>
> I think len can be declared int16_t to avoid using wrong values?
> (here and where you call this function)
>
>> +     hdr->nexthdr = proto;
>> +     hdr->hop_limit = np->hop_limit;
>> +
>> +     hdr->saddr = *ipv6_src;
>> +     hdr->daddr = *ipv6_dst;
>> +}
>> +
>> +/**
>> + * batadv_ndisc_create_na - create an NA for a solicited NS
>> + * @net_device: the devices for which the packet is created
>> + * @ipv6_dst: destination IPv6
>> + * @ipv6_target: target IPv6 (same with source IPv6)
>> + * @hw_dst: destination HW Address
>> + * @hw_target: target HW Address (same with source HW Address)
>> + * @router: 1 if target is a router, else 0
>> + * @solicited: 1 if this is a solicited NA, else 0
>> + * @override: 1 if the target entry should be override, else 0
>> + *
>> + * For more info see RFC2461.
>
> If you want to cite the RFC think you should provide also a section?
> The "Neighbor Discovery for IP Version 6" RFC is a bit too much is I only want
> to understand what a NA or NS is and how the NA is created.
>
> By the way, ok for reading the RFC, but I think you can write a bit more about
> what the function is doing: e.g. create a new skb containing an NA which fields
> are initialised with the value passed as argument. Seems obvious, but better
> safe than sorry :) Kernel Doc is shown without the code if you compile it.
>
>> + *
>> + * Returns the newly created skb, otherwise NULL.
>> + */
>> +static struct
>> +sk_buff *batadv_ndisc_create_na(struct net_device *dev,
>> +                             const struct in6_addr *ipv6_dst,
>> +                             const struct in6_addr *ipv6_target,
>> +                             const uint8_t *hw_dst,
>> +                             const uint8_t *hw_target,
>> +                             int router, int solicited, int override)
>
> if these variables can only be 0 or 1, why not using bool?
>
>> +{
>> +     struct net *net = dev_net(dev);
>> +     struct sock *sk = net->ipv6.ndisc_sk;
>> +     struct sk_buff *skb;
>> +     struct icmp6hdr *icmp6hdr;
>> +     int hlen = LL_RESERVED_SPACE(dev);
>> +     int tlen = dev->needed_tailroom;
>> +     int len, err;
>> +     u8 *opt;
>
> uint8_t
>
>> +
>> +     /* alloc space for skb */
>> +     len = sizeof(struct icmp6hdr) + sizeof(*ipv6_target) + 8;
>
> write in the comment what is this 8. Otherwise, since you use it more than once,
> create a define with a descriptive name and it instead of the 8.
>
>> +     skb = sock_alloc_send_skb(sk,
>> +                               (MAX_HEADER + sizeof(struct ipv6hdr) +
>
>                                         why these parenthesis here?
>
>> +                                len + hlen + tlen),
>
> I guess hlen is ETH_HLEN? what does LL_RESERVED_SPACE exactly return?
> and why using needed_tailroom? what does it comport in this case?
> We never used that during SKB allocation...this is why I am asking.
>
>> +                               1, &err);
>
> and why are you using this function to allocate the skb? In the rest of the code
> we always use netdev_alloc_skb_ip_align() (that also takes care of properly
> aligning the skb).
>
>> +     if (!skb)
>> +             return NULL;
>> +
>> +     skb_reserve(skb, hlen);
>> +     batadv_ndisc_ip6_hdr(sk, skb, dev, ipv6_target, ipv6_dst,
>> +                          IPPROTO_ICMPV6, len);
>> +
>> +     skb->transport_header = skb->tail;
>
> why you care about setting the transport header?
> You could also use skb_set_transport_header() passing a proper offset rather
> than directly using skb->tail. Following the skb logic is "sometimes" not
> straightforward, therefore it is better to use the provided functions when
> possible.
>
>> +     skb_put(skb, len);
>> +
>> +     /* fill the device header for the NA frame */
>> +     if (dev_hard_header(skb, dev, ETH_P_IPV6, hw_dst,
>> +                         hw_target, skb->len) < 0) {
>> +             kfree_skb(skb);
>> +             return NULL;
>> +     }
>
> mh..we never used this function as we assume that the interface below batman-adv
> is carrying 802.3 frame only. But looks interesting :)
>
>> +
>> +     /* set icmpv6 header */
>> +     icmp6hdr = (struct icmp6hdr *)skb_transport_header(skb);
>
> ah now I understood why you have set the transport_header :)
>
>> +     icmp6hdr->icmp6_type = NDISC_NEIGHBOUR_ADVERTISEMENT;
>> +     icmp6hdr->icmp6_router = router;
>> +     icmp6hdr->icmp6_solicited = solicited;
>> +     icmp6hdr->icmp6_override = override;
>> +
>> +     /* set NA target and options */
>> +     opt = skb_transport_header(skb) + sizeof(*icmp6hdr);
>> +     *(struct in6_addr *)opt = *ipv6_target;
>> +     opt += sizeof(*ipv6_target);
>> +
>> +     opt[0] = ND_OPT_TARGET_LL_ADDR;
>> +     opt[1] = 1;
>> +     memcpy(opt + 2, hw_target, dev->addr_len);
>
> ah, these are the famous 8 bytes :) So hard to discover! :D
>
>> +
>> +     icmp6hdr->icmp6_cksum = csum_ipv6_magic(ipv6_target, ipv6_dst, len,
>> +                                             IPPROTO_ICMPV6,
>> +                                             csum_partial(icmp6hdr, len, 0));
>> +
>> +     return skb;
>> +}
>> +#endif
>> +
>>  /**
>>   * batadv_dat_snoop_outgoing_pkt_request - snoop the ARP request and try to
>>   * answer using DAT
>> --
>> 1.7.10.4
>
> --
> Antonio Quartulli
>
> ..each of us alone is worth nothing..
> Ernesto "Che" Guevara

I will resolve most of the comments when I redo the patch.

Thanks,
Mihail