| Message ID | 20201014115805.23905-1-ceggers@arri.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: ptp: get rid of IPV4_HLEN() and OFF_IHL macros | expand |
On Wed, Oct 14, 2020 at 01:58:05PM +0200, Christian Eggers wrote: > Both macros are already marked for removal. I'm not sure what Daniel Borkmann meant by that comment, but ... > switch (type & PTP_CLASS_PMASK) { > case PTP_CLASS_IPV4: > - ptr += IPV4_HLEN(ptr) + UDP_HLEN; > + ptr += (((struct iphdr *)ptr)->ihl << 2) + UDP_HLEN; to my eyes IPV4_HLEN(ptr) is way more readable than (((struct iphdr *)ptr)->ihl << 2) and this (struct udphdr *)((char *)ih + (ih->ihl << 2)) is really baroque. I don't see any improvement here. Thanks, Richard
On 10/14/20 8:36 PM, Richard Cochran wrote: > On Wed, Oct 14, 2020 at 01:58:05PM +0200, Christian Eggers wrote: >> Both macros are already marked for removal. > > I'm not sure what Daniel Borkmann meant by that comment, but ... > >> switch (type & PTP_CLASS_PMASK) { >> case PTP_CLASS_IPV4: >> - ptr += IPV4_HLEN(ptr) + UDP_HLEN; >> + ptr += (((struct iphdr *)ptr)->ihl << 2) + UDP_HLEN; > > to my eyes > > IPV4_HLEN(ptr) > > is way more readable than > > (((struct iphdr *)ptr)->ihl << 2) > > and this > > (struct udphdr *)((char *)ih + (ih->ihl << 2)) > > is really baroque. > > I don't see any improvement here. Having recently helped someone with a bug that involved using IPV4_HLEN() instead of ip_hdr() in a driver's transmit path (so with the transport header correctly set), it would probably help if we could make IPV4_HLEN()'s semantics clearer with converting it to a static inline function and put asserts in there about what the transport and MAC header positions should be. At the very least, creating a new function, like this maybe in include/linux/ip.h might help: static inline u8 __ip_hdr_len(const struct sk_buff *skb) { const struct iphdr *ip_hdr = (const struct iphdr *)(skb->data); return ip_hdr->ihl << 2; }
On Thursday, 15 October 2020, 18:56:41 CEST, Florian Fainelli wrote: > Having recently helped someone with a bug that involved using > IPV4_HLEN() instead of ip_hdr() in a driver's transmit path (so with the > transport header correctly set), it would probably help if we could make > IPV4_HLEN()'s semantics clearer with converting it to a static inline > function and put asserts in there about what the transport and MAC > header positions should be. IPV4_HLEN() is only used for PTP. Is there any way to use "normal" infrastructure as in the rest of the network stack? It looks like PTP code typically has to look into multiple network layers (mac, network, transport) at places these layers may not be processed yet (at least in RX direction). > At the very least, creating a new function, like this maybe in > include/linux/ip.h might help: > > static inline u8 __ip_hdr_len(const struct sk_buff *skb) > { > const struct iphdr *ip_hdr = (const struct iphdr *)(skb->data); > > return ip_hdr->ihl << 2; > } Is there any reason using skb->data instead of skb_network_header()? Debugging through my DSA driver showed that ... - for TX (called by dsa_slave_xmit), skb->data is set to the MAC header (skb->head+0x02), whilst skb->network_header is correctly set to 0x10 (skb->mac_header+14). - for TX, skb->transport_header is 0xffff, so udp_hdr() cannot be used - for RX (called by dsa_switch_rcv), skb->data is set to skb->head+0x50, which is identical to skb->network_header - for RX, skb->transport_header ist set equal to skb->network_header, so udp_hdr() can also not be used. Best regards Christian
On Fri, Oct 16, 2020 at 09:04:38AM +0200, Christian Eggers wrote: > IPV4_HLEN() is only used for PTP. Is there any way to use "normal" > infrastructure as in the rest of the network stack? You answered your own question... > It looks like PTP code > typically has to look into multiple network layers (mac, network, transport) > at places these layers may not be processed yet (at least in RX direction). here! (The pointers in the skb are moved around as the skb flows through the stack.) The original, per-driver, un-refactored callers of that macro (at least the ones written by me) used the macro carefully. Also, I think the recent re-factoring that Kurt implemented preserves the correct usage of the macro. Any new users must, of course, use it carefully while parsing the bytes of a frame. I agree that the macro is perhaps not the best possible solution, but it is not the worst either. Thanks, Richard
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ptp.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ptp.c index 70dbee89118e..b32a9006b222 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ptp.c +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ptp.c @@ -83,8 +83,8 @@ bool is_ptp_enabled(struct sk_buff *skb, struct net_device *dev) */ bool cxgb4_ptp_is_ptp_rx(struct sk_buff *skb) { - struct udphdr *uh = (struct udphdr *)(skb->data + ETH_HLEN + - IPV4_HLEN(skb->data)); + struct iphdr *ih = (struct iphdr *)(skb->data + ETH_HLEN); + struct udphdr *uh = (struct udphdr *)((char *)ih + (ih->ihl << 2)); return uh->dest == htons(PTP_EVENT_PORT) && uh->source == htons(PTP_EVENT_PORT); diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c index a9e9c7ae565d..c8bec874bc66 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/sge.c +++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c @@ -40,6 +40,7 @@ #include <linux/dma-mapping.h> #include <linux/jiffies.h> #include <linux/prefetch.h> +#include <linux/ptp_classify.h> #include <linux/export.h> #include <net/xfrm.h> #include <net/ipv6.h> @@ -3386,7 +3387,9 @@ static noinline int t4_systim_to_hwstamp(struct adapter *adapter, data = skb->data + sizeof(*cpl); skb_pull(skb, 2 * sizeof(u64) + sizeof(struct cpl_rx_mps_pkt)); - offset = ETH_HLEN + IPV4_HLEN(skb->data) + UDP_HLEN; + offset = ETH_HLEN; + offset += ((struct iphdr *)(skb->data + offset))->ihl << 2; + offset += UDP_HLEN; if (skb->len < offset + OFF_PTP_SEQUENCE_ID + sizeof(short)) return RX_PTP_PKT_ERR; diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c index ade8c44c01cd..4e95621997d1 100644 --- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c +++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c @@ -113,7 +113,9 @@ static int pch_ptp_match(struct sk_buff *skb, u16 uid_hi, u32 uid_lo, u16 seqid) if (ptp_classify_raw(skb) == PTP_CLASS_NONE) return 0; - offset = ETH_HLEN + IPV4_HLEN(data) + UDP_HLEN; + offset = ETH_HLEN; + offset += ((struct iphdr *)(data + offset))->ihl << 2; + offset += UDP_HLEN; if (skb->len < offset + OFF_PTP_SEQUENCE_ID + sizeof(seqid)) return 0; diff --git a/drivers/net/ethernet/xscale/ixp4xx_eth.c b/drivers/net/ethernet/xscale/ixp4xx_eth.c index 2e5202923510..7443bc1f9bec 100644 --- a/drivers/net/ethernet/xscale/ixp4xx_eth.c +++ b/drivers/net/ethernet/xscale/ixp4xx_eth.c @@ -264,7 +264,9 @@ static int ixp_ptp_match(struct sk_buff *skb, u16 uid_hi, u32 uid_lo, u16 seqid) if (ptp_classify_raw(skb) != PTP_CLASS_V1_IPV4) return 0; - offset = ETH_HLEN + IPV4_HLEN(data) + UDP_HLEN; + offset = ETH_HLEN; + offset += ((struct iphdr *)(data + offset))->ihl << 2; + offset += UDP_HLEN; if (skb->len < offset + OFF_PTP_SEQUENCE_ID + sizeof(seqid)) return 0; diff --git a/include/linux/ptp_classify.h b/include/linux/ptp_classify.h index c6487b7ab026..56b2d7d66177 100644 --- a/include/linux/ptp_classify.h +++ b/include/linux/ptp_classify.h @@ -40,8 +40,6 @@ /* Below defines should actually be removed at some point in time. */ #define IP6_HLEN 40 #define UDP_HLEN 8 -#define OFF_IHL 14 -#define IPV4_HLEN(data) (((struct iphdr *)(data + OFF_IHL))->ihl << 2) struct clock_identity { u8 id[8]; diff --git a/net/core/ptp_classifier.c b/net/core/ptp_classifier.c index e33fde06d528..6a964639b704 100644 --- a/net/core/ptp_classifier.c +++ b/net/core/ptp_classifier.c @@ -114,9 +114,11 @@ struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type) if (type & PTP_CLASS_VLAN) ptr += VLAN_HLEN; + ptr += ETH_HLEN; + switch (type & PTP_CLASS_PMASK) { case PTP_CLASS_IPV4: - ptr += IPV4_HLEN(ptr) + UDP_HLEN; + ptr += (((struct iphdr *)ptr)->ihl << 2) + UDP_HLEN; break; case PTP_CLASS_IPV6: ptr += IP6_HLEN + UDP_HLEN; @@ -127,8 +129,6 @@ struct ptp_header *ptp_parse_header(struct sk_buff *skb, unsigned int type) return NULL; } - ptr += ETH_HLEN; - /* Ensure that the entire header is present in this packet. */ if (ptr + sizeof(struct ptp_header) > skb->data + skb->len) return NULL;
Both macros are already marked for removal. IPV4_HLEN(data) is misleading as it expects an Ethernet header instead of an IPv4 header as argument. Because it is defined (and only used) within PTP, it should be named PTP_IPV4_HLEN or similar. As the whole rest of the IPv4 stack has no problems using iphdr->ihl directly, also PTP should be able to do so. OFF_IHL has only been used by IPV4_HLEN. Additionally it is superfluous as ETH_HLEN already exists for the same. Signed-off-by: Christian Eggers <ceggers@arri.de> --- drivers/net/ethernet/chelsio/cxgb4/cxgb4_ptp.c | 4 ++-- drivers/net/ethernet/chelsio/cxgb4/sge.c | 5 ++++- drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 4 +++- drivers/net/ethernet/xscale/ixp4xx_eth.c | 4 +++- include/linux/ptp_classify.h | 2 -- net/core/ptp_classifier.c | 6 +++--- 6 files changed, 15 insertions(+), 10 deletions(-)