netdev - Re: [RFC 1/3] networking: make skb_put & friends return void pointers

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:   Fri, 16 Jun 2017 09:53:11 +0200
From:   Bjørn Mork <bjorn@...k.no>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     netdev@...r.kernel.org, Johannes Berg <johannes.berg@...el.com>
Subject: Re: [RFC 1/3] networking: make skb_put & friends return void pointers

Johannes Berg <johannes@...solutions.net> writes:

> From: Johannes Berg <johannes.berg@...el.com>
>
> It seems like a historic accident that these return unsigned char *,
> and in many places that means casts are required, more often than not.
>
> Make these functions (skb_put, __skb_put and pskb_put) return void *
> and remove all the casts across the tree, adding a (u8 *) cast only
> where the unsigned char pointer was used directly, all done with the
> following spatch:
>
>     @@
>     expression SKB, LEN;
>     typedef u8;
>     identifier fn = { skb_put, __skb_put };
>     @@
>     - *(fn(SKB, LEN))
>     + *(u8 *)fn(SKB, LEN)
>
>     @@


There seem to be a large number of places where the char pointer was
used directly. Not that I have any strong opinion either way, but adding
lots of ugly casts like this seems to contradict the whole purpose of
this change?:

>  			if (info->rx_count == 0) {
> diff --git a/drivers/bluetooth/bt3c_cs.c b/drivers/bluetooth/bt3c_cs.c
> index 8165ef2fe877..be2d431aa366 100644
> --- a/drivers/bluetooth/bt3c_cs.c
> +++ b/drivers/bluetooth/bt3c_cs.c
> @@ -282,7 +282,7 @@ static void bt3c_receive(struct bt3c_info *info)
>  
>  			__u8 x = inb(iobase + DATA_L);
>  
> -			*skb_put(info->rx_skb, 1) = x;
> +			*(u8 *)skb_put(info->rx_skb, 1) = x;
>  			inb(iobase + DATA_H);
>  			info->rx_count--;
>  

That does not look any better in my eyes, and there are ... what?
... hundreds of them?


No complaints about the other skb_* changes you are doing, Those are
nice cleanups.  But skb_put is different IMHO.


Bjørn