netdev - Re: [PATCH net-next 1/6] net: qualcomm: rmnet: Remove the rmnet_map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 05 Dec 2017 11:53:40 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     subashab@...eaurora.org
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/6] net: qualcomm: rmnet: Remove the
 rmnet_map_results enum

From: Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>
Date: Sun,  3 Dec 2017 23:37:03 -0700

> Only the success and consumed entries were actually in use.
> Use standard error codes instead.
> 
> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>

That is definitely not the only thing this change is doing:

> @@ -126,12 +126,12 @@ static int rmnet_map_egress_handler(struct sk_buff *skb,
>  
>  	if (skb_headroom(skb) < required_headroom) {
>  		if (pskb_expand_head(skb, required_headroom, 0, GFP_KERNEL))
> -			return RMNET_MAP_CONSUMED;
> +			goto fail;
>  	}
>  
>  	map_header = rmnet_map_add_map_header(skb, additional_header_len, 0);
>  	if (!map_header)
> -		return RMNET_MAP_CONSUMED;
> +		goto fail;
>  

Previously these paths would return "RMNET_MAP_CONSUMED":

> @@ -209,17 +213,8 @@ void rmnet_egress_handler(struct sk_buff *skb)
>  	}
>  
>  	if (port->egress_data_format & RMNET_EGRESS_FORMAT_MAP) {
> -		switch (rmnet_map_egress_handler(skb, port, mux_id, orig_dev)) {
> -		case RMNET_MAP_CONSUMED:
> -			return;
> -
> -		case RMNET_MAP_SUCCESS:
> -			break;
> -
> -		default:
> -			kfree_skb(skb);
> +		if (rmnet_map_egress_handler(skb, port, mux_id, orig_dev))
>  			return;
> -		}
>  	}

Causing this code to return.

Now, instead the code jumps to fail:

> @@ -142,7 +142,11 @@ static int rmnet_map_egress_handler(struct sk_buff *skb,
>  
>  	skb->protocol = htons(ETH_P_MAP);
>  
> -	return RMNET_MAP_SUCCESS;
> +	return 0;
> +
> +fail:
> +	kfree_skb(skb);
> +	return -ENOMEM;
>  }
>  

Which frees the SKB.

This is a behavioral change, perhaps a bug fix.

Well, nobody knows because you do not explain this at all in your
commit message.

Do not mix functional changes with cleanups.  If you want to change how
freeing the SKB is done, do it in a separate change from the patch that
removes this enumeration.

Thank you.