[PATCH net-next] tcp: tcp_sendmsg() wrong access to sk_route_caps

[PATCH net-next] tcp: tcp_sendmsg() wrong access to sk_route_caps

[Eric Dumazet]

Now sk_route_caps is u64, its dangerous to use an integer to store
result of an AND operator. It wont work if NETIF_F_SG is moved on the
upper part of u64.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 net/ipv4/tcp.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 50c3596..ecbc89a 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -891,7 +891,7 @@ EXPORT_SYMBOL(tcp_sendpage);
 #define TCP_PAGE(sk)	(sk->sk_sndmsg_page)
 #define TCP_OFF(sk)	(sk->sk_sndmsg_off)
 
-static inline int select_size(const struct sock *sk, int sg)
+static inline int select_size(const struct sock *sk, bool sg)
 {
 	const struct tcp_sock *tp = tcp_sk(sk);
 	int tmp = tp->mss_cache;
@@ -917,9 +917,9 @@ int tcp_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
 	struct iovec *iov;
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct sk_buff *skb;
-	int iovlen, flags;
+	int iovlen, flags, err, copied;
 	int mss_now, size_goal;
-	int sg, err, copied;
+	bool sg;
 	long timeo;
 
 	lock_sock(sk);
@@ -946,7 +946,7 @@ int tcp_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
 	if (sk->sk_err || (sk->sk_shutdown & SEND_SHUTDOWN))
 		goto out_err;
 
-	sg = sk->sk_route_caps & NETIF_F_SG;
+	sg = !!(sk->sk_route_caps & NETIF_F_SG);
 
 	while (--iovlen >= 0) {
 		size_t seglen = iov->iov_len;

Re: [PATCH net-next] tcp: tcp_sendmsg() wrong access to sk_route_caps

[Joe Perches]

On Mon, 2011-11-28 at 11:27 +0100, Eric Dumazet wrote:
> Now sk_route_caps is u64, its dangerous to use an integer to store
> result of an AND operator. It wont work if NETIF_F_SG is moved on the
> upper part of u64.

trivial comment below.

> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
[]
> @@ -917,9 +917,9 @@ int tcp_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
>  	struct iovec *iov;
>  	struct tcp_sock *tp = tcp_sk(sk);
>  	struct sk_buff *skb;
> -	int iovlen, flags;
> +	int iovlen, flags, err, copied;
>  	int mss_now, size_goal;
> -	int sg, err, copied;
> +	bool sg;
>  	long timeo;
>  
>  	lock_sock(sk);
> @@ -946,7 +946,7 @@ int tcp_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
>  	if (sk->sk_err || (sk->sk_shutdown & SEND_SHUTDOWN))
>  		goto out_err;
>  
> -	sg = sk->sk_route_caps & NETIF_F_SG;
> +	sg = !!(sk->sk_route_caps & NETIF_F_SG);

As sg is now bool, using !! is unnecessary.

A commit was done recently to remove one.
3ad9b358e03fd9dbf6705721490c811b666b0fe2

Re: [PATCH net-next] tcp: tcp_sendmsg() wrong access to sk_route_caps

[Eric Dumazet]

Le lundi 28 novembre 2011 à 02:46 -0800, Joe Perches a écrit :
> On Mon, 2011-11-28 at 11:27 +0100, Eric Dumazet wrote:
> > Now sk_route_caps is u64, its dangerous to use an integer to store
> > result of an AND operator. It wont work if NETIF_F_SG is moved on the
> > upper part of u64.
> 
> trivial comment below.

Well, not trivial at all ;)

> 
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> []
> > @@ -917,9 +917,9 @@ int tcp_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
> >  	struct iovec *iov;
> >  	struct tcp_sock *tp = tcp_sk(sk);
> >  	struct sk_buff *skb;
> > -	int iovlen, flags;
> > +	int iovlen, flags, err, copied;
> >  	int mss_now, size_goal;
> > -	int sg, err, copied;
> > +	bool sg;
> >  	long timeo;
> >  
> >  	lock_sock(sk);
> > @@ -946,7 +946,7 @@ int tcp_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
> >  	if (sk->sk_err || (sk->sk_shutdown & SEND_SHUTDOWN))
> >  		goto out_err;
> >  
> > -	sg = sk->sk_route_caps & NETIF_F_SG;
> > +	sg = !!(sk->sk_route_caps & NETIF_F_SG);
> 
> As sg is now bool, using !! is unnecessary.
> 
> A commit was done recently to remove one.
> 3ad9b358e03fd9dbf6705721490c811b666b0fe2
> 

Hmm... I find it dangerous and error prone. Obviously not at the time we
commit such changes, but later, because a future reader might be fooled.

Using !!(expr) is pretty clear about the potential problem, and
generates no extra code if a bool is used for the target.

RE: [PATCH net-next] tcp: tcp_sendmsg() wrong access to sk_route_caps

[David Laight]

 
> > > -	sg = sk->sk_route_caps & NETIF_F_SG;
> > > +	sg = !!(sk->sk_route_caps & NETIF_F_SG);
> > 
> > As sg is now bool, using !! is unnecessary.
> > 
> > A commit was done recently to remove one.
> > 3ad9b358e03fd9dbf6705721490c811b666b0fe2
> > 
> 
> Hmm... I find it dangerous and error prone. Obviously not at 
> the time we
> commit such changes, but later, because a future reader might 
> be fooled.
> 
> Using !!(expr) is pretty clear about the potential problem, and
> generates no extra code if a bool is used for the target.

Or leave 'sg' as 'unsigned int' and add:
    COMPILE_ASSERT(NETIF_F_SG & ~0);
(or however linux spells the appropriate define)

	David

Re: [PATCH net-next] tcp: tcp_sendmsg() wrong access to sk_route_caps

[Joe Perches]

On Mon, 2011-11-28 at 17:04 +0100, Eric Dumazet wrote:
> Le lundi 28 novembre 2011 à 02:46 -0800, Joe Perches a écrit :
> > On Mon, 2011-11-28 at 11:27 +0100, Eric Dumazet wrote:
> > > +	sg = !!(sk->sk_route_caps & NETIF_F_SG);
> > As sg is now bool, using !! is unnecessary.
> > A commit was done recently to remove one.
> > 3ad9b358e03fd9dbf6705721490c811b666b0fe2
> Hmm... I find it dangerous and error prone. Obviously not at the time we
> commit such changes, but later, because a future reader might be fooled.
> Using !!(expr) is pretty clear about the potential problem, and
> generates no extra code if a bool is used for the target.

Though I don't think
	bool = expr;
is particularly error prone,
spatch shows 58 uses of
	bool = !!expr;
in net-next drivers/net.

It might be useful to standardize on either the
implicit or explicit !! style.

I think !! is an intentional and sensible style
and should be the preferred use.

Perhaps a checkpatch warning should be issued
when the implicit cast is used.

Re: [PATCH net-next] tcp: tcp_sendmsg() wrong access to sk_route_caps

[Ben Hutchings]

On Mon, 2011-11-28 at 08:54 -0800, Joe Perches wrote:
> On Mon, 2011-11-28 at 17:04 +0100, Eric Dumazet wrote:
> > Le lundi 28 novembre 2011 à 02:46 -0800, Joe Perches a écrit :
> > > On Mon, 2011-11-28 at 11:27 +0100, Eric Dumazet wrote:
> > > > +	sg = !!(sk->sk_route_caps & NETIF_F_SG);
> > > As sg is now bool, using !! is unnecessary.
> > > A commit was done recently to remove one.
> > > 3ad9b358e03fd9dbf6705721490c811b666b0fe2
> > Hmm... I find it dangerous and error prone. Obviously not at the time we
> > commit such changes, but later, because a future reader might be fooled.
> > Using !!(expr) is pretty clear about the potential problem, and
> > generates no extra code if a bool is used for the target.
> 
> Though I don't think
> 	bool = expr;
> is particularly error prone,
> spatch shows 58 uses of
> 	bool = !!expr;
> in net-next drivers/net.
> 
> It might be useful to standardize on either the
> implicit or explicit !! style.
> 
> I think !! is an intentional and sensible style
> and should be the preferred use.

I see it as a workaround for the historical lack of a real bool type in
C.

> Perhaps a checkpatch warning should be issued
> when the implicit cast is used.

checkpatch can't do type-checking; maybe you mean sparse.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next] tcp: tcp_sendmsg() wrong access to sk_route_caps

[Joe Perches]

On Mon, 2011-11-28 at 17:20 +0000, Ben Hutchings wrote:
> On Mon, 2011-11-28 at 08:54 -0800, Joe Perches wrote:
> > Perhaps a checkpatch warning should be issued
> > when the implicit cast [to bool] is used.
> checkpatch can't do type-checking; maybe you mean sparse.

I think I could get checkpatch to track the bool
type implicit casts for the not struct member
uses of bool.

ie:
	bool foo;
	[...]
	foo = expr;
but not:
	struct foo {
		bool bar;
	};
	[...]
	foo.bar = expr;

It's pretty trivial to do it mostly correctly
with spatch/cocci though.

Re: [PATCH net-next] tcp: tcp_sendmsg() wrong access to sk_route_caps

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 28 Nov 2011 11:27:47 +0100

> Now sk_route_caps is u64, its dangerous to use an integer to store
> result of an AND operator. It wont work if NETIF_F_SG is moved on the
> upper part of u64.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Applied, thanks Eric.