Re: [PATCH] net: use skb_is_gso_sctp() instead of open-coding

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Fri, Mar 09, 2018 at 02:06:09PM +1100, Daniel Axtens wrote:
> As well as the basic conversion, I noticed that a lot of the
> SCTP code checks gso_type without first checking skb_is_gso()
> so I have added that where appropriate.
> 
> Also, document the helper.

Thanks Daniel.

> 
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> 
> ---
> 
> This depends on d02f51cbcf12 ("bpf: fix bpf_skb_adjust_net/bpf_skb_proto_xlat
> to deal with gso sctp skbs") which introduces the required helper.
> That is in the bpf tree at the moment.
> ---
>  Documentation/networking/segmentation-offloads.txt | 5 ++++-
>  net/core/skbuff.c                                  | 2 +-
>  net/sched/act_csum.c                               | 2 +-
>  net/sctp/input.c                                   | 8 ++++----
>  net/sctp/inqueue.c                                 | 2 +-
>  net/sctp/offload.c                                 | 2 +-
>  6 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/networking/segmentation-offloads.txt b/Documentation/networking/segmentation-offloads.txt
> index 23a8dd91a9ec..36bb931b35e0 100644
> --- a/Documentation/networking/segmentation-offloads.txt
> +++ b/Documentation/networking/segmentation-offloads.txt
> @@ -155,7 +155,10 @@ Therefore, any code in the core networking stack must be aware of the
>  possibility that gso_size will be GSO_BY_FRAGS and handle that case
>  appropriately.
>  
> -There are a couple of helpers to make this easier:
> +There are some helpers to make this easier:
> +
> + - skb_is_gso(skb) && skb_is_gso_sctp(skb) is the best way to see if
> +   an skb is an SCTP GSO skb.
>  
>   - For size checks, the skb_gso_validate_*_len family of helpers correctly
>     considers GSO_BY_FRAGS.
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 0bb0d8877954..baf990528943 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4904,7 +4904,7 @@ static unsigned int skb_gso_transport_seglen(const struct sk_buff *skb)
>  			thlen += inner_tcp_hdrlen(skb);
>  	} else if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))) {
>  		thlen = tcp_hdrlen(skb);
> -	} else if (unlikely(shinfo->gso_type & SKB_GSO_SCTP)) {
> +	} else if (unlikely(skb_is_gso_sctp(skb))) {
>  		thlen = sizeof(struct sctphdr);
>  	}
>  	/* UFO sets gso_size to the size of the fragmentation
> diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
> index b7ba9b06b147..24b2e8e681cf 100644
> --- a/net/sched/act_csum.c
> +++ b/net/sched/act_csum.c
> @@ -350,7 +350,7 @@ static int tcf_csum_sctp(struct sk_buff *skb, unsigned int ihl,
>  {
>  	struct sctphdr *sctph;
>  
> -	if (skb_is_gso(skb) && skb_shinfo(skb)->gso_type & SKB_GSO_SCTP)
> +	if (skb_is_gso(skb) && skb_is_gso_sctp(skb))
>  		return 1;
>  
>  	sctph = tcf_csum_skb_nextlayer(skb, ihl, ipl, sizeof(*sctph));
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index 0247cc432e02..b381d78548ac 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -106,6 +106,7 @@ int sctp_rcv(struct sk_buff *skb)
>  	int family;
>  	struct sctp_af *af;
>  	struct net *net = dev_net(skb->dev);
> +	bool is_gso = skb_is_gso(skb) && skb_is_gso_sctp(skb);
>  
>  	if (skb->pkt_type != PACKET_HOST)
>  		goto discard_it;
> @@ -123,8 +124,7 @@ int sctp_rcv(struct sk_buff *skb)
>  	 * it's better to just linearize it otherwise crc computing
>  	 * takes longer.
>  	 */
> -	if ((!(skb_shinfo(skb)->gso_type & SKB_GSO_SCTP) &&
> -	     skb_linearize(skb)) ||
> +	if ((!is_gso && skb_linearize(skb)) ||
>  	    !pskb_may_pull(skb, sizeof(struct sctphdr)))
>  		goto discard_it;
>  
> @@ -135,7 +135,7 @@ int sctp_rcv(struct sk_buff *skb)
>  	if (skb_csum_unnecessary(skb))
>  		__skb_decr_checksum_unnecessary(skb);
>  	else if (!sctp_checksum_disable &&
> -		 !(skb_shinfo(skb)->gso_type & SKB_GSO_SCTP) &&
> +		 !is_gso &&
>  		 sctp_rcv_checksum(net, skb) < 0)
>  		goto discard_it;
>  	skb->csum_valid = 1;
> @@ -1218,7 +1218,7 @@ static struct sctp_association *__sctp_rcv_lookup_harder(struct net *net,
>  	 * issue as packets hitting this are mostly INIT or INIT-ACK and
>  	 * those cannot be on GSO-style anyway.
>  	 */
> -	if ((skb_shinfo(skb)->gso_type & SKB_GSO_SCTP) == SKB_GSO_SCTP)
> +	if (skb_is_gso(skb) && skb_is_gso_sctp(skb))
>  		return NULL;
>  
>  	ch = (struct sctp_chunkhdr *)skb->data;
> diff --git a/net/sctp/inqueue.c b/net/sctp/inqueue.c
> index 48392552ee7c..23ebc5318edc 100644
> --- a/net/sctp/inqueue.c
> +++ b/net/sctp/inqueue.c
> @@ -170,7 +170,7 @@ struct sctp_chunk *sctp_inq_pop(struct sctp_inq *queue)
>  
>  		chunk = list_entry(entry, struct sctp_chunk, list);
>  
> -		if ((skb_shinfo(chunk->skb)->gso_type & SKB_GSO_SCTP) == SKB_GSO_SCTP) {
> +		if (skb_is_gso(chunk->skb) && skb_is_gso_sctp(chunk->skb)) {
>  			/* GSO-marked skbs but without frags, handle
>  			 * them normally
>  			 */
> diff --git a/net/sctp/offload.c b/net/sctp/offload.c
> index 35bc7106d182..123e9f2dc226 100644
> --- a/net/sctp/offload.c
> +++ b/net/sctp/offload.c
> @@ -45,7 +45,7 @@ static struct sk_buff *sctp_gso_segment(struct sk_buff *skb,
>  	struct sk_buff *segs = ERR_PTR(-EINVAL);
>  	struct sctphdr *sh;
>  
> -	if (!(skb_shinfo(skb)->gso_type & SKB_GSO_SCTP))
> +	if (!skb_is_gso_sctp(skb))
>  		goto out;
>  
>  	sh = sctp_hdr(skb);
> -- 
> 2.14.1
>