re: xprtrdma: Move credit update to RPC reply handler

re: xprtrdma: Move credit update to RPC reply handler

[Dan Carpenter]

Hello Chuck Lever,

The patch eba8ff660b2d: "xprtrdma: Move credit update to RPC reply
handler" from Jan 21, 2015, leads to the following static checker
warning:

	net/sunrpc/xprtrdma/rpc_rdma.c:879 rpcrdma_reply_handler()
	warn: can 'credits' be negative?

net/sunrpc/xprtrdma/rpc_rdma.c
   875  
   876          credits = be32_to_cpu(headerp->rm_credit);
   877          if (credits == 0)
   878                  credits = 1;    /* don't deadlock */
   879          else if (credits > r_xprt->rx_buf.rb_max_requests)
   880                  credits = r_xprt->rx_buf.rb_max_requests;


"credits" and "r_xprt->rx_buf.rb_max_requests" are both type int so this
test can underflow.  If "credits" is less than zero then "xprt->cwnd"
could be zero, leading to a deadlock (based on the comment), or
something else out of bounds.

   881  
   882          cwnd = xprt->cwnd;
   883          xprt->cwnd = credits << RPC_CWNDSHIFT;
   884          if (xprt->cwnd > cwnd)
   885                  xprt_release_rqst_cong(rqst->rq_task);
   886  
   887          dprintk("RPC:       %s: xprt_complete_rqst(0x%p, 0x%p, %d)\n",
   888                          __func__, xprt, rqst, status);
   889          xprt_complete_rqst(rqst->rq_task, status);
   890          spin_unlock(&xprt->transport_lock);
   891  }

regards,
dan carpenter

Re: xprtrdma: Move credit update to RPC reply handler

[Chuck Lever]

On Feb 10, 2015, at 4:45 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:

> Hello Chuck Lever,
> 
> The patch eba8ff660b2d: "xprtrdma: Move credit update to RPC reply
> handler" from Jan 21, 2015, leads to the following static checker
> warning:
> 
> 	net/sunrpc/xprtrdma/rpc_rdma.c:879 rpcrdma_reply_handler()
> 	warn: can 'credits' be negative?

“credits” cannot be negative unless there’s a bug.

I can send an additional patch once this is committed (so the correct
commit ID is available for the Fixes: tag).


> net/sunrpc/xprtrdma/rpc_rdma.c
>   875  
>   876          credits = be32_to_cpu(headerp->rm_credit);
>   877          if (credits == 0)
>   878                  credits = 1;    /* don't deadlock */
>   879          else if (credits > r_xprt->rx_buf.rb_max_requests)
>   880                  credits = r_xprt->rx_buf.rb_max_requests;
> 
> 
> "credits" and "r_xprt->rx_buf.rb_max_requests" are both type int so this
> test can underflow.  If "credits" is less than zero then "xprt->cwnd"
> could be zero, leading to a deadlock (based on the comment), or
> something else out of bounds.
> 
>   881  
>   882          cwnd = xprt->cwnd;
>   883          xprt->cwnd = credits << RPC_CWNDSHIFT;
>   884          if (xprt->cwnd > cwnd)
>   885                  xprt_release_rqst_cong(rqst->rq_task);
>   886  
>   887          dprintk("RPC:       %s: xprt_complete_rqst(0x%p, 0x%p, %d)\n",
>   888                          __func__, xprt, rqst, status);
>   889          xprt_complete_rqst(rqst->rq_task, status);
>   890          spin_unlock(&xprt->transport_lock);
>   891  }
> 
> regards,
> dan carpenter

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com