Re: [PATCH v2 4/4] net: make skb_gso_*_seglen functions private

[Marcelo Ricardo Leitner]

On Thu, Mar 01, 2018 at 05:13:40PM +1100, Daniel Axtens wrote:
> They're very hard to use properly as they do not consider the
> GSO_BY_FRAGS case. Code should use skb_gso_validate_network_len
> and skb_gso_validate_mac_len as they do consider this case.
> 
> Make the seglen functions static, which stops people using them
> outside of skbuff.c
> 
> Signed-off-by: Daniel Axtens 

Reviewed-by: Marcelo Ricardo Leitner 

> 
> ---
> 
> v2: drop inline from functions.
> ---
>  include/linux/skbuff.h | 33 -
>  net/core/skbuff.c  | 37 +++--
>  2 files changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index a057dd1a75c7..ddf77cf4ff2d 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3285,7 +3285,6 @@ int skb_zerocopy(struct sk_buff *to, struct sk_buff 
> *from,
>  void skb_split(struct sk_buff *skb, struct sk_buff *skb1, const u32 len);
>  int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen);
>  void skb_scrub_packet(struct sk_buff *skb, bool xnet);
> -unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
>  bool skb_gso_validate_network_len(const struct sk_buff *skb, unsigned int 
> mtu);
>  bool skb_gso_validate_mac_len(const struct sk_buff *skb, unsigned int len);
>  struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
> @@ -4104,38 +4103,6 @@ static inline bool skb_head_is_locked(const struct 
> sk_buff *skb)
>   return !skb->head_frag || skb_cloned(skb);
>  }
>  
> -/**
> - * skb_gso_network_seglen - Return length of individual segments of a gso 
> packet
> - *
> - * @skb: GSO skb
> - *
> - * skb_gso_network_seglen is used to determine the real size of the
> - * individual segments, including Layer3 (IP, IPv6) and L4 headers (TCP/UDP).
> - *
> - * The MAC/L2 header is not accounted for.
> - */
> -static inline unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
> -{
> - unsigned int hdr_len = skb_transport_header(skb) -
> -skb_network_header(skb);
> - return hdr_len + skb_gso_transport_seglen(skb);
> -}
> -
> -/**
> - * skb_gso_mac_seglen - Return length of individual segments of a gso packet
> - *
> - * @skb: GSO skb
> - *
> - * skb_gso_mac_seglen is used to determine the real size of the
> - * individual segments, including MAC/L2, Layer3 (IP, IPv6) and L4
> - * headers (TCP/UDP).
> - */
> -static inline unsigned int skb_gso_mac_seglen(const struct sk_buff *skb)
> -{
> - unsigned int hdr_len = skb_transport_header(skb) - skb_mac_header(skb);
> - return hdr_len + skb_gso_transport_seglen(skb);
> -}
> -
>  /* Local Checksum Offload.
>   * Compute outer checksum based on the assumption that the
>   * inner checksum will be offloaded later.
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index b63767008824..0bb0d8877954 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4891,7 +4891,7 @@ EXPORT_SYMBOL_GPL(skb_scrub_packet);
>   *
>   * The MAC/L2 or network (IP, IPv6) headers are not accounted for.
>   */
> -unsigned int skb_gso_transport_seglen(const struct sk_buff *skb)
> +static unsigned int skb_gso_transport_seglen(const struct sk_buff *skb)
>  {
>   const struct skb_shared_info *shinfo = skb_shinfo(skb);
>   unsigned int thlen = 0;
> @@ -4913,7 +4913,40 @@ unsigned int skb_gso_transport_seglen(const struct 
> sk_buff *skb)
>*/
>   return thlen + shinfo->gso_size;
>  }
> -EXPORT_SYMBOL_GPL(skb_gso_transport_seglen);
> +
> +/**
> + * skb_gso_network_seglen - Return length of individual segments of a gso 
> packet
> + *
> + * @skb: GSO skb
> + *
> + * skb_gso_network_seglen is used to determine the real size of the
> + * individual segments, including Layer3 (IP, IPv6) and L4 headers (TCP/UDP).
> + *
> + * The MAC/L2 header is not accounted for.
> + */
> +static unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
> +{
> + unsigned int hdr_len = skb_transport_header(skb) -
> +skb_network_header(skb);
> +
> + return hdr_len + skb_gso_transport_seglen(skb);
> +}
> +
> +/**
> + * skb_gso_mac_seglen - Return length of individual segments of a gso packet
> + *
> + * @skb: GSO skb
> + *
> + * skb_gso_mac_seglen is used to determine the real size of the
> + * individual segments, including MAC/L2, Layer3 (IP, IPv6) and L4
> + * headers (TCP/UDP).
> + */
> +static unsigned int skb_gso_mac_seglen(const struct sk_buff *skb)
> +{
> + unsigned int hdr_len = skb_transport_header(skb) - skb_mac_header(skb);
> +
> + return hdr_len + skb_gso_transport_seglen(skb);
> +}
>  
>  /**
>   * skb_gso_size_check - check the skb size, considering GSO_BY_FRAGS
> -- 
> 2.14.1
> 

[PATCH v2 4/4] net: make skb_gso_*_seglen functions private

[Daniel Axtens]

They're very hard to use properly as they do not consider the
GSO_BY_FRAGS case. Code should use skb_gso_validate_network_len
and skb_gso_validate_mac_len as they do consider this case.

Make the seglen functions static, which stops people using them
outside of skbuff.c

Signed-off-by: Daniel Axtens 

---

v2: drop inline from functions.
---
 include/linux/skbuff.h | 33 -
 net/core/skbuff.c  | 37 +++--
 2 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a057dd1a75c7..ddf77cf4ff2d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3285,7 +3285,6 @@ int skb_zerocopy(struct sk_buff *to, struct sk_buff *from,
 void skb_split(struct sk_buff *skb, struct sk_buff *skb1, const u32 len);
 int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen);
 void skb_scrub_packet(struct sk_buff *skb, bool xnet);
-unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
 bool skb_gso_validate_network_len(const struct sk_buff *skb, unsigned int mtu);
 bool skb_gso_validate_mac_len(const struct sk_buff *skb, unsigned int len);
 struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
@@ -4104,38 +4103,6 @@ static inline bool skb_head_is_locked(const struct 
sk_buff *skb)
return !skb->head_frag || skb_cloned(skb);
 }
 
-/**
- * skb_gso_network_seglen - Return length of individual segments of a gso 
packet
- *
- * @skb: GSO skb
- *
- * skb_gso_network_seglen is used to determine the real size of the
- * individual segments, including Layer3 (IP, IPv6) and L4 headers (TCP/UDP).
- *
- * The MAC/L2 header is not accounted for.
- */
-static inline unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
-{
-   unsigned int hdr_len = skb_transport_header(skb) -
-  skb_network_header(skb);
-   return hdr_len + skb_gso_transport_seglen(skb);
-}
-
-/**
- * skb_gso_mac_seglen - Return length of individual segments of a gso packet
- *
- * @skb: GSO skb
- *
- * skb_gso_mac_seglen is used to determine the real size of the
- * individual segments, including MAC/L2, Layer3 (IP, IPv6) and L4
- * headers (TCP/UDP).
- */
-static inline unsigned int skb_gso_mac_seglen(const struct sk_buff *skb)
-{
-   unsigned int hdr_len = skb_transport_header(skb) - skb_mac_header(skb);
-   return hdr_len + skb_gso_transport_seglen(skb);
-}
-
 /* Local Checksum Offload.
  * Compute outer checksum based on the assumption that the
  * inner checksum will be offloaded later.
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index b63767008824..0bb0d8877954 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4891,7 +4891,7 @@ EXPORT_SYMBOL_GPL(skb_scrub_packet);
  *
  * The MAC/L2 or network (IP, IPv6) headers are not accounted for.
  */
-unsigned int skb_gso_transport_seglen(const struct sk_buff *skb)
+static unsigned int skb_gso_transport_seglen(const struct sk_buff *skb)
 {
const struct skb_shared_info *shinfo = skb_shinfo(skb);
unsigned int thlen = 0;
@@ -4913,7 +4913,40 @@ unsigned int skb_gso_transport_seglen(const struct 
sk_buff *skb)
 */
return thlen + shinfo->gso_size;
 }
-EXPORT_SYMBOL_GPL(skb_gso_transport_seglen);
+
+/**
+ * skb_gso_network_seglen - Return length of individual segments of a gso 
packet
+ *
+ * @skb: GSO skb
+ *
+ * skb_gso_network_seglen is used to determine the real size of the
+ * individual segments, including Layer3 (IP, IPv6) and L4 headers (TCP/UDP).
+ *
+ * The MAC/L2 header is not accounted for.
+ */
+static unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
+{
+   unsigned int hdr_len = skb_transport_header(skb) -
+  skb_network_header(skb);
+
+   return hdr_len + skb_gso_transport_seglen(skb);
+}
+
+/**
+ * skb_gso_mac_seglen - Return length of individual segments of a gso packet
+ *
+ * @skb: GSO skb
+ *
+ * skb_gso_mac_seglen is used to determine the real size of the
+ * individual segments, including MAC/L2, Layer3 (IP, IPv6) and L4
+ * headers (TCP/UDP).
+ */
+static unsigned int skb_gso_mac_seglen(const struct sk_buff *skb)
+{
+   unsigned int hdr_len = skb_transport_header(skb) - skb_mac_header(skb);
+
+   return hdr_len + skb_gso_transport_seglen(skb);
+}
 
 /**
  * skb_gso_size_check - check the skb size, considering GSO_BY_FRAGS
-- 
2.14.1