[v2,2/7] batman-adv: speed up dat by snooping received ip traffic
Commit Message
Speeding up dat address lookup is achieved by snooping all incoming ip
traffic. This especially increases the propability in bla setups that
a gateway into a common backbone network already has a fitting dat entry
to answer incoming ARP requests directly coming from the backbone
network thus further reducing ARP traffic in the mesh.
Signed-off-by: Andreas Pape <apape@phoenixcontact.com>
---
net/batman-adv/distributed-arp-table.c | 18 ++++++++++++++++++
net/batman-adv/distributed-arp-table.h | 9 ++++++++-
net/batman-adv/soft-interface.c | 22 +++++++++++++++++++++-
3 files changed, 47 insertions(+), 2 deletions(-)
--
1.7.0.4
..................................................................
PHOENIX CONTACT ELECTRONICS GmbH
Sitz der Gesellschaft / registered office of the company: 31812 Bad Pyrmont
USt-Id-Nr.: DE811742156
Amtsgericht Hannover HRB 100528 / district court Hannover HRB 100528
Geschäftsführer / Executive Board: Roland Bent, Dr. Martin Heubeck
Comments
On Friday 26 February 2016 14:18:17 Andreas Pape wrote:
> Speeding up dat address lookup is achieved by snooping all incoming ip
> traffic. This especially increases the propability in bla setups that
> a gateway into a common backbone network already has a fitting dat entry
> to answer incoming ARP requests directly coming from the backbone
> network thus further reducing ARP traffic in the mesh.
>
> Signed-off-by: Andreas Pape <apape@phoenixcontact.com>
> ---
[...]
> switch (ntohs(ethhdr->h_proto)) {
> + case ETH_P_IP:
> + 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);
> + break;
> case ETH_P_8021Q:
> vhdr = (struct vlan_ethhdr *)skb->data;
>
> - if (vhdr->h_vlan_encapsulated_proto != ethertype)
> + if (vhdr->h_vlan_encapsulated_proto != ethertype) {
> + /* snoop incoming traffic for dat update also for vlan
> + * tagged frames.
> + */
> + if (vhdr->h_vlan_encapsulated_proto == ETH_P_IP) {
> + iphdr = (struct iphdr *)(vhdr +
> + sizeof(struct vlan_ethhdr));
> + batadv_dat_entry_check(bat_priv, iphdr->saddr,
> + vhdr->h_source, vid);
> + }
Where is the code to check that there is enough data for the iphdr
(pskb_may_pull)? Same question goes to Marek for his initial change in
48628bb9419f ("batman-adv: softif bridge loop avoidance") that introduced the
vhdr->vlan_ethhdr check
Kind regards,
Sven
On Friday 26 February 2016 14:18:17 Andreas Pape wrote:
> + iphdr = (struct iphdr *)(vhdr +
> + sizeof(struct vlan_ethhdr));
Forgot this one: sizeof(*vhdr) == 18. So you basically calculated
(u8 *)vhdr + 18 * 18
But I would guess that you actually wanted to calculate
(u8 *)vhdr + 18
Kind regards,
Sven
On Friday 26 February 2016 14:18:17 Andreas Pape wrote:
> + if (vhdr->h_vlan_encapsulated_proto == ETH_P_IP) {
Either you htons the ETH_P_IP or you have to ntohs the
vhdr->h_vlan_encapsulated_proto. But one of them has to converted to the byte
order of the other one.
Kind regards,
Sven
Hello Sven,
Sven Eckelmann <sven@narfation.org> schrieb am 26.02.2016 17:00:49:
> Von: Sven Eckelmann <sven@narfation.org>
> An: b.a.t.m.a.n@lists.open-mesh.org
> Kopie: Andreas Pape <apape@phoenixcontact.com>, Marek Lindner
> <lindner_marek@yahoo.de>
> Datum: 26.02.2016 17:00
> Betreff: Re: [B.A.T.M.A.N.] [PATCHv2 2/7] batman-adv: speed up dat
> by snooping received ip traffic
>
> On Friday 26 February 2016 14:18:17 Andreas Pape wrote:
> > Speeding up dat address lookup is achieved by snooping all incoming ip
> > traffic. This especially increases the propability in bla setups that
> > a gateway into a common backbone network already has a fitting dat
entry
> > to answer incoming ARP requests directly coming from the backbone
> > network thus further reducing ARP traffic in the mesh.
> >
> > Signed-off-by: Andreas Pape <apape@phoenixcontact.com>
> > ---
> [...]
> > switch (ntohs(ethhdr->h_proto)) {
> > + case ETH_P_IP:
> > + 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);
> > + break;
> > case ETH_P_8021Q:
> > vhdr = (struct vlan_ethhdr *)skb->data;
> >
> > - if (vhdr->h_vlan_encapsulated_proto != ethertype)
> > + if (vhdr->h_vlan_encapsulated_proto != ethertype) {
> > + /* snoop incoming traffic for dat update also for vlan
> > + * tagged frames.
> > + */
> > + if (vhdr->h_vlan_encapsulated_proto == ETH_P_IP) {
> > + iphdr = (struct iphdr *)(vhdr +
> > + sizeof(struct vlan_ethhdr));
> > + batadv_dat_entry_check(bat_priv, iphdr->saddr,
> > + vhdr->h_source, vid);
> > + }
>
> Where is the code to check that there is enough data for the iphdr
> (pskb_may_pull)? Same question goes to Marek for his initial change in
> 48628bb9419f ("batman-adv: softif bridge loop avoidance") that
introduced the
> vhdr->vlan_ethhdr check
>
I will leave this to the result of the discussion in [1] which you started
and will not handle this in the next version of my patchset. Correct?
Kind regards,
Andreas
[1]
https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2016-February/014506.html
..................................................................
PHOENIX CONTACT ELECTRONICS GmbH
Sitz der Gesellschaft / registered office of the company: 31812 Bad Pyrmont
USt-Id-Nr.: DE811742156
Amtsgericht Hannover HRB 100528 / district court Hannover HRB 100528
Geschäftsführer / Executive Board: Roland Bent, Dr. Martin Heubeck
On Tuesday 01 March 2016 16:52:47 Andreas Pape wrote:
[...]
> > Where is the code to check that there is enough data for the iphdr
> > (pskb_may_pull)? Same question goes to Marek for his initial change in
> > 48628bb9419f ("batman-adv: softif bridge loop avoidance") that
> introduced the
> > vhdr->vlan_ethhdr check
> >
>
> I will leave this to the result of the discussion in [1] which you started
> and will not handle this in the next version of my patchset. Correct?
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.
Btw. you can skip this check when you just use skb_header_pointer to access
the IPv4 header. It will return NULL when the data cannot be accessed. It
will also take care of copying data out of the fragments in your temporary
"buffer" when the bytes are not in the main buffer of the skb. You just have
to prepare a large enough buffer on the stack and only use the returned
pointer.
Kind regards,
Sven
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);
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);
}
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)?
> Btw. you can skip this check when you just use skb_header_pointer to
access
> the IPv4 header. It will return NULL when the data cannot be accessed.
It
> will also take care of copying data out of the fragments in your
temporary
> "buffer" when the bytes are not in the main buffer of the skb. You just
have
> to prepare a large enough buffer on the stack and only use the returned
> pointer.
>
Kind regards,
Andreas
..................................................................
PHOENIX CONTACT ELECTRONICS GmbH
Sitz der Gesellschaft / registered office of the company: 31812 Bad Pyrmont
USt-Id-Nr.: DE811742156
Amtsgericht Hannover HRB 100528 / district court Hannover HRB 100528
Geschäftsführer / Executive Board: Roland Bent, Dr. Martin Heubeck
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.
Kind regards,
Sven
Sven Eckelmann <sven@narfation.org> schrieb am 02.03.2016 11:23:21:
> > 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 :)
Ooops.. Of course you are right about not dropping the packet here!
Further more
I will follow your suggestion to move this into a separate function to not
mix two different things.
Best regards,
Andreas
..................................................................
PHOENIX CONTACT ELECTRONICS GmbH
Sitz der Gesellschaft / registered office of the company: 31812 Bad Pyrmont
USt-Id-Nr.: DE811742156
Amtsgericht Hannover HRB 100528 / district court Hannover HRB 100528
Geschäftsführer / Executive Board: Roland Bent, Dr. Martin Heubeck
@@ -362,6 +362,24 @@ out:
batadv_dat_entry_put(dat_entry);
}
+/**
+ * batadv_dat_entry_check - check and update a dat entry
+ * @bat_priv: the bat priv with all the soft interface information
+ * @ip: ipv4 to add/edit
+ * @mac_addr: mac address to assign to the given ipv4
+ * @vid: VLAN identifier
+ *
+ * checks additionally, if dat is enabled. can be called from other modules.
+ */
+void batadv_dat_entry_check(struct batadv_priv *bat_priv, __be32 ip,
+ u8 *mac_addr, unsigned short vid)
+{
+ if (!atomic_read(&bat_priv->distributed_arp_table))
+ return;
+
+ batadv_dat_entry_add(bat_priv, ip, mac_addr, vid);
+}
+
#ifdef CONFIG_BATMAN_ADV_DEBUG
/**
@@ -80,7 +80,8 @@ batadv_dat_init_own_addr(struct batadv_priv *bat_priv,
int batadv_dat_init(struct batadv_priv *bat_priv);
void batadv_dat_free(struct batadv_priv *bat_priv);
int batadv_dat_cache_seq_print_text(struct seq_file *seq, void *offset);
-
+void batadv_dat_entry_check(struct batadv_priv *bat_priv, __be32 ip,
+ u8 *mac_addr, unsigned short vid);
/**
* batadv_dat_inc_counter - increment the correct DAT packet counter
* @bat_priv: the bat priv with all the soft interface information
@@ -173,6 +174,12 @@ static inline void batadv_dat_inc_counter(struct batadv_priv *bat_priv,
{
}
+static inline
+void batadv_dat_entry_check(struct batadv_priv *bat_priv, __be32 ip,
+ u8 *mac_addr, unsigned short vid)
+{
+}
+
#endif /* CONFIG_BATMAN_ADV_DAT */
#endif /* _NET_BATMAN_ADV_DISTRIBUTED_ARP_TABLE_H_ */
@@ -28,6 +28,7 @@
#include <linux/fs.h>
#include <linux/if_ether.h>
#include <linux/if_vlan.h>
+#include <linux/ip.h>
#include <linux/jiffies.h>
#include <linux/kernel.h>
#include <linux/kref.h>
@@ -390,6 +391,7 @@ void batadv_interface_rx(struct net_device *soft_iface,
__be16 ethertype = htons(ETH_P_BATMAN);
struct vlan_ethhdr *vhdr;
struct ethhdr *ethhdr;
+ struct iphdr *iphdr;
unsigned short vid;
bool is_bcast;
@@ -412,11 +414,29 @@ void batadv_interface_rx(struct net_device *soft_iface,
ethhdr = eth_hdr(skb);
switch (ntohs(ethhdr->h_proto)) {
+ case ETH_P_IP:
+ 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);
+ break;
case ETH_P_8021Q:
vhdr = (struct vlan_ethhdr *)skb->data;
- if (vhdr->h_vlan_encapsulated_proto != ethertype)
+ if (vhdr->h_vlan_encapsulated_proto != ethertype) {
+ /* snoop incoming traffic for dat update also for vlan
+ * tagged frames.
+ */
+ if (vhdr->h_vlan_encapsulated_proto == ETH_P_IP) {
+ iphdr = (struct iphdr *)(vhdr +
+ sizeof(struct vlan_ethhdr));
+ batadv_dat_entry_check(bat_priv, iphdr->saddr,
+ vhdr->h_source, vid);
+ }
break;
+ }
/* fall through */
case ETH_P_BATMAN: