Re: [PATCHv2 1/1] net: rds: add service level support in rds-info

[Zhu Yanjun <yanjun.zhu@xxxxxxxxxx>]

On 2019/9/3 9:58, Gustavo A. R. Silva wrote:
> Hi,
>
> On 8/23/19 8:04 PM, Zhu Yanjun wrote:
>
> [..]
>
> > diff --git a/net/rds/ib.c b/net/rds/ib.c
> > index ec05d91..45acab2 100644
> > --- a/net/rds/ib.c
> > +++ b/net/rds/ib.c
> > @@ -291,7 +291,7 @@ static int rds_ib_conn_info_visitor(struct rds_connection *conn,
> >   				    void *buffer)
> >   {
> >   	struct rds_info_rdma_connection *iinfo = buffer;
> > -	struct rds_ib_connection *ic;
> > +	struct rds_ib_connection *ic = conn->c_transport_data;
> >   
> >   	/* We will only ever look at IB transports */
> >   	if (conn->c_trans != &rds_ib_transport)
> > @@ -301,15 +301,16 @@ static int rds_ib_conn_info_visitor(struct rds_connection *conn,
> >   
> >   	iinfo->src_addr = conn->c_laddr.s6_addr32[3];
> >   	iinfo->dst_addr = conn->c_faddr.s6_addr32[3];
> > -	iinfo->tos = conn->c_tos;
> > +	if (ic) {
> Is this null-check actually necessary? (see related comments below...)
>
> > +		iinfo->tos = conn->c_tos;
> > +		iinfo->sl = ic->i_sl;
> > +	}
> >   
> >   	memset(&iinfo->src_gid, 0, sizeof(iinfo->src_gid));
> >   	memset(&iinfo->dst_gid, 0, sizeof(iinfo->dst_gid));
> >   	if (rds_conn_state(conn) == RDS_CONN_UP) {
> >   		struct rds_ib_device *rds_ibdev;
> >   
> > -		ic = conn->c_transport_data;
> > -
> >   		rdma_read_gids(ic->i_cm_id, (union ib_gid *)&iinfo->src_gid,
> Notice that *ic* is dereferenced here without null-checking it. More
> comments below...
>
> >   			       (union ib_gid *)&iinfo->dst_gid);
> >   
> > @@ -329,7 +330,7 @@ static int rds6_ib_conn_info_visitor(struct rds_connection *conn,
> >   				     void *buffer)
> >   {
> >   	struct rds6_info_rdma_connection *iinfo6 = buffer;
> > -	struct rds_ib_connection *ic;
> > +	struct rds_ib_connection *ic = conn->c_transport_data;
> >   
> >   	/* We will only ever look at IB transports */
> >   	if (conn->c_trans != &rds_ib_transport)
> > @@ -337,6 +338,10 @@ static int rds6_ib_conn_info_visitor(struct rds_connection *conn,
> >   
> >   	iinfo6->src_addr = conn->c_laddr;
> >   	iinfo6->dst_addr = conn->c_faddr;
> > +	if (ic) {
> > +		iinfo6->tos = conn->c_tos;
> > +		iinfo6->sl = ic->i_sl;
> > +	}
> >   
> >   	memset(&iinfo6->src_gid, 0, sizeof(iinfo6->src_gid));
> >   	memset(&iinfo6->dst_gid, 0, sizeof(iinfo6->dst_gid));
> > @@ -344,7 +349,6 @@ static int rds6_ib_conn_info_visitor(struct rds_connection *conn,
> >   	if (rds_conn_state(conn) == RDS_CONN_UP) {
> >   		struct rds_ib_device *rds_ibdev;
> >   
> > -		ic = conn->c_transport_data;
> >   		rdma_read_gids(ic->i_cm_id, (union ib_gid *)&iinfo6->src_gid,
> Again, *ic* is being dereferenced here without a previous null-check.

Please  check when this "rds_conn_state(conn) = RDS_CONN_UP".

Thanks a lot.

Zhu Yanjun

> >   			       (union ib_gid *)&iinfo6->dst_gid);
> >   		rds_ibdev = ic->rds_ibdev;
>
> --
> Gustavo