On 1/29/2020 10:11 PM, Jason Gunthorpe wrote: > On Wed, Jan 29, 2020 at 02:43:51PM +0200, Gal Pressman wrote: >> On 29/01/2020 14:14, Maor Gottlieb wrote: >>> On 1/29/2020 2:06 PM, Gal Pressman wrote: >>>> On 26/01/2020 19:15, Leon Romanovsky wrote: >>>>> From: Maor Gottlieb <maorg@xxxxxxxxxxxx> >>>>> >>>>> We don't need to set pkey as valid in case that user set only one >>>>> of pkey index or port number, otherwise it will be resulted in NULL >>>>> pointer dereference while accessing to uninitialized pkey list. >>>> Why would the pkey list be uninitialized? Isn't it initialized as an empty list >>>> on device registration? >>> It will try to access to list of invalid port / pkey, e.g. to list of >>> port 0. port_data is indexed by port number. >>> dev->port_data[pp->port_num].pkey_list >> Makes sense. >> Shouldn't there be a check in the (!qp_pps) section as well? We shouldn't assign >> the field unless the mask is given. > Indeed, reading a qp_attr field without the corresponding bt in > qp_attr_mask set should be wrong. Agree, but it doesn't affect cause we don't use this value if the pkey is not valid. However I can add it. > >> Does this work correctly if the user issues two calls to modify_qp where the >> first one modifies the pkey index and the second the port number (if that's even >> possible)? >> Is it expected that the state would stay invalid? > Also sounds wrong When QP is modified from reset to init, both pkey and port should be set, but this check happens later when the vendor call to ib_modify_qp_is_ok. > > .. and then there is the confusing testing of state != > IB_PORT_PKEY_NOT_VALID but nothing ever assigns > IB_PORT_PKEY_NOT_VALID.. Humm. IB_PORT_PKEY_NOT_VALID is the default value, set when we do kzalloc. > > Jason
