[B.A.T.M.A.N.] [PATCH] batman-adv: access h_vlan_encapsulated_proto properly

[B.A.T.M.A.N.] [PATCH] batman-adv: access h_vlan_encapsulated_proto properly

[Antonio Quartulli]

In the gateway code in case of VLAN tagged frame the ethhdr
pointer is moved forward by 4 bytes so that the offset of
h_proto in struct ethhdr matches the real
h_vlan_encapsulated_proto address in the skb.

This trick is correct but the code is not easy to understand
and may lead to bugs in case of re-use of ethhdr for other
purposes.

Use a standalone protocol variable and assign it accordingly
to the header type

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
 gateway_client.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/gateway_client.c b/gateway_client.c
index 973f02d..8ea13e1 100644
--- a/gateway_client.c
+++ b/gateway_client.c
@@ -626,23 +626,29 @@ bool batadv_gw_is_dhcp_target(struct sk_buff *skb, unsigned int *header_len)
 	struct iphdr *iphdr;
 	struct ipv6hdr *ipv6hdr;
 	struct udphdr *udphdr;
+	uint16_t proto;
 
 	/* check for ethernet header */
 	if (!pskb_may_pull(skb, *header_len + ETH_HLEN))
 		return false;
 	ethhdr = (struct ethhdr *)skb->data;
+	proto = ntohs(ethhdr->h_proto);
 	*header_len += ETH_HLEN;
 
 	/* check for initial vlan header */
-	if (ntohs(ethhdr->h_proto) == ETH_P_8021Q) {
+	if (proto == ETH_P_8021Q) {
+		struct vlan_ethhdr *vhdr;
+
 		if (!pskb_may_pull(skb, *header_len + VLAN_HLEN))
 			return false;
-		ethhdr = (struct ethhdr *)(skb->data + VLAN_HLEN);
+
+		vhdr = (struct vlan_ethhdr *)skb->data;
+		proto = ntohs(vhdr->h_vlan_encapsulated_proto);
 		*header_len += VLAN_HLEN;
 	}
 
 	/* check for ip header */
-	switch (ntohs(ethhdr->h_proto)) {
+	switch (proto) {
 	case ETH_P_IP:
 		if (!pskb_may_pull(skb, *header_len + sizeof(*iphdr)))
 			return false;
@@ -675,11 +681,11 @@ bool batadv_gw_is_dhcp_target(struct sk_buff *skb, unsigned int *header_len)
 	*header_len += sizeof(*udphdr);
 
 	/* check for bootp port */
-	if ((ntohs(ethhdr->h_proto) == ETH_P_IP) &&
+	if ((proto == ETH_P_IP) &&
 	    (ntohs(udphdr->dest) != 67))
 		return false;
 
-	if ((ntohs(ethhdr->h_proto) == ETH_P_IPV6) &&
+	if ((proto == ETH_P_IPV6) &&
 	    (ntohs(udphdr->dest) != 547))
 		return false;
 
-- 
1.8.1.5

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: access h_vlan_encapsulated_proto properly

[Marek Lindner]

On Tuesday, May 07, 2013 19:54:02 Antonio Quartulli wrote:
> In the gateway code in case of VLAN tagged frame the ethhdr
> pointer is moved forward by 4 bytes so that the offset of
> h_proto in struct ethhdr matches the real
> h_vlan_encapsulated_proto address in the skb.
> 
> This trick is correct but the code is not easy to understand
> and may lead to bugs in case of re-use of ethhdr for other
> purposes.
> 
> Use a standalone protocol variable and assign it accordingly
> to the header type

How about adding the key word "refactoring" somewhere into the subject line or 
the commit message ?


> @@ -626,23 +626,29 @@ bool batadv_gw_is_dhcp_target(struct sk_buff *skb,
> unsigned int *header_len) struct iphdr *iphdr;
>  	struct ipv6hdr *ipv6hdr;
>  	struct udphdr *udphdr;
> +	uint16_t proto;
> 
>  	/* check for ethernet header */
>  	if (!pskb_may_pull(skb, *header_len + ETH_HLEN))
>  		return false;
>  	ethhdr = (struct ethhdr *)skb->data;
> +	proto = ntohs(ethhdr->h_proto);
>  	*header_len += ETH_HLEN;
> 
>  	/* check for initial vlan header */
> -	if (ntohs(ethhdr->h_proto) == ETH_P_8021Q) {
> +	if (proto == ETH_P_8021Q) {
> +		struct vlan_ethhdr *vhdr;
> +

While we are refactoring I suggest to not ntohs() the protocol every time but 
the protocol constants. The compiler will replace the constants with the 
appropriate number during the compilation, thus saving us from a runtime 
operation every time we check a packet.

Cheers,
Marek

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: access h_vlan_encapsulated_proto properly

[Antonio Quartulli]

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

On Sun, May 12, 2013 at 03:25:10PM +0800, Marek Lindner wrote:
> > On Tuesday, May 07, 2013 19:54:02 Antonio Quartulli wrote:
> > > In the gateway code in case of VLAN tagged frame the ethhdr
> > > pointer is moved forward by 4 bytes so that the offset of
> > > h_proto in struct ethhdr matches the real
> > > h_vlan_encapsulated_proto address in the skb.
> > > 
> > > This trick is correct but the code is not easy to understand
> > > and may lead to bugs in case of re-use of ethhdr for other
> > > purposes.
> > > 
> > > Use a standalone protocol variable and assign it accordingly
> > > to the header type
> > 
> > How about adding the key word "refactoring" somewhere into the subject line or 
> > the commit message ?
> 


oky


> 
> > 
> > 
> > > @@ -626,23 +626,29 @@ bool batadv_gw_is_dhcp_target(struct sk_buff *skb,
> > > unsigned int *header_len) struct iphdr *iphdr;
> > >  	struct ipv6hdr *ipv6hdr;
> > >  	struct udphdr *udphdr;
> > > +	uint16_t proto;
> > > 
> > >  	/* check for ethernet header */
> > >  	if (!pskb_may_pull(skb, *header_len + ETH_HLEN))
> > >  		return false;
> > >  	ethhdr = (struct ethhdr *)skb->data;
> > > +	proto = ntohs(ethhdr->h_proto);
> > >  	*header_len += ETH_HLEN;
> > > 
> > >  	/* check for initial vlan header */
> > > -	if (ntohs(ethhdr->h_proto) == ETH_P_8021Q) {
> > > +	if (proto == ETH_P_8021Q) {
> > > +		struct vlan_ethhdr *vhdr;
> > > +
> > 
> > While we are refactoring I suggest to not ntohs() the protocol every time but 
> > the protocol constants. The compiler will replace the constants with the 
> > appropriate number during the compilation, thus saving us from a runtime 
> > operation every time we check a packet.
> > 
> 

oh right. I did not think about it. I guess I have to use __constant_htons
instead of the plain hntons.

cheers,


 -- 
 Antonio Quartulli

 ..each of us alone is worth nothing..
 Ernesto "Che" Guevara



-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]