From: Antonio Quartulli <ordex@autistici.org>
To: The list for a Better Approach To Mobile Ad-hoc Networking
<b.a.t.m.a.n@lists.open-mesh.org>
Subject: Re: [B.A.T.M.A.N.] [RFC 4/6] batman-adv: Adds necessary functions for NDP, like checking if a packet is valid or creating a Neighbor Advertisement
Date: Sat, 10 Aug 2013 14:20:55 +0200 [thread overview]
Message-ID: <20130810122055.GC849@ritirata.org> (raw)
In-Reply-To: <1373242365-763-4-git-send-email-mihail.costea2005@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 15370 bytes --]
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
--
Antonio Quartulli
..each of us alone is worth nothing..
Ernesto "Che" Guevara
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
next prev parent reply other threads:[~2013-08-10 12:20 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-08 0:12 [B.A.T.M.A.N.] [RFC 1/6] batman-adv: Generalize DAT in order to support any type of data, not only IPv4 mihail.costea2005
2013-07-08 0:12 ` [B.A.T.M.A.N.] [RFC 2/6] batman-adv: Renames batadv_dat_snoop_*_arp_* functions to batadv_dat_snoop_*_pkt_* mihail.costea2005
2013-07-08 0:12 ` [B.A.T.M.A.N.] [RFC 3/6] batman-adv: Adds IPv6 to DAT and generic struct in distributed-arp-table.c mihail.costea2005
2013-08-10 11:14 ` Antonio Quartulli
2013-07-08 0:12 ` [B.A.T.M.A.N.] [RFC 4/6] batman-adv: Adds necessary functions for NDP, like checking if a packet is valid or creating a Neighbor Advertisement mihail.costea2005
2013-08-10 12:20 ` Antonio Quartulli [this message]
2013-08-14 13:38 ` Mihail Costea
2013-07-08 0:12 ` [B.A.T.M.A.N.] [RFC 5/6] batman-adv: Generalize snooping mechanism in order to suport NDP too mihail.costea2005
2013-09-30 20:06 ` Linus Lüssing
2013-09-30 20:38 ` Linus Lüssing
2013-10-04 18:28 ` Mihail Costea
2013-10-05 7:14 ` Antonio Quartulli
2013-10-05 9:48 ` Mihail Costea
2013-07-08 0:12 ` [B.A.T.M.A.N.] [RFC 6/6] batman-adv: Adds snooping of router and override flags for NA creation mihail.costea2005
2013-08-10 13:20 ` Antonio Quartulli
2013-08-14 13:51 ` Mihail Costea
2013-08-14 15:42 ` Linus Lüssing
2013-08-14 17:42 ` Antonio Quartulli
2013-07-23 7:27 ` [B.A.T.M.A.N.] [RFC 1/6] batman-adv: Generalize DAT in order to support any type of data, not only IPv4 Antonio Quartulli
2013-07-24 16:50 ` Mihail Costea
2013-08-10 11:03 ` Antonio Quartulli
2013-08-10 19:01 ` Mihail Costea
2013-08-10 20:36 ` Antonio Quartulli
2013-09-09 14:05 ` Mihail Costea
2013-09-09 14:53 ` Antonio Quartulli
2013-09-10 4:35 ` Mihail Costea
2013-09-10 5:38 ` Antonio Quartulli
2013-09-10 12:45 ` Mihail Costea
2013-09-10 21:01 ` Antonio Quartulli
2013-09-11 4:33 ` Mihail Costea
2013-09-11 6:46 ` Antonio Quartulli
2016-03-10 19:11 ` Sven Eckelmann
2016-03-20 12:02 ` Antonio Quartulli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130810122055.GC849@ritirata.org \
--to=ordex@autistici.org \
--cc=b.a.t.m.a.n@lists.open-mesh.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).