RE: [PATCH RESEND] RDMA/siw: Remove direct link to net_device

[Bernard Metzler]

> -----Original Message-----
> From: Bernard Metzler <bmt@xxxxxxxxxxxxxx>
> Sent: Thursday, December 12, 2024 3:36 PM
> To: linux-rdma@xxxxxxxxxxxxxxx
> Cc: jgg@xxxxxxxx; leon@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; syzkaller-bugs@xxxxxxxxxxxxxxxx;
> zyjzyj2000@xxxxxxxxx; Bernard Metzler <BMT@xxxxxxxxxxxxxx>;
> syzbot+4b87489410b4efd181bf@xxxxxxxxxxxxxxxxxxxxxxxxx
> Subject: [PATCH RESEND] RDMA/siw: Remove direct link to net_device
> 
> Do not manage a per device direct link to net_device. Rely
> on associated ib_devices net_device management, not doubling
> the effort locally. A badly managed local link to net_device
> was causing a 'KASAN: slab-use-after-free' exception during
> siw_query_port() call.
> 
> Fixes: bdcf26bf9b3a ("rdma/siw: network and RDMA core interface")
> Reported-by: syzbot+4b87489410b4efd181bf@xxxxxxxxxxxxxxxxxxxxxxxxx
> Link: https://syzkaller.appspot.com/bug?extid=4b87489410b4efd181bf
> Signed-off-by: Bernard Metzler <bmt@xxxxxxxxxxxxxx>
> ---
>  drivers/infiniband/sw/siw/siw.h       |  7 +++---
>  drivers/infiniband/sw/siw/siw_cm.c    | 31 +++++++++++++++++++++------
>  drivers/infiniband/sw/siw/siw_main.c  | 15 +------------
>  drivers/infiniband/sw/siw/siw_verbs.c | 27 +++++++++++++++++------
>  4 files changed, 50 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/siw/siw.h
> b/drivers/infiniband/sw/siw/siw.h
> index 86d4d6a2170e..ea5eee50dc39 100644
> --- a/drivers/infiniband/sw/siw/siw.h
> +++ b/drivers/infiniband/sw/siw/siw.h
> @@ -46,6 +46,9 @@
>   */
>  #define SIW_IRQ_MAXBURST_SQ_ACTIVE 4
> 
> +/* There is always only a port 1 per siw device */
> +#define SIW_PORT 1
> +
>  struct siw_dev_cap {
>  	int max_qp;
>  	int max_qp_wr;
> @@ -69,16 +72,12 @@ struct siw_pd {
> 
>  struct siw_device {
>  	struct ib_device base_dev;
> -	struct net_device *netdev;
>  	struct siw_dev_cap attrs;
> 
>  	u32 vendor_part_id;
>  	int numa_node;
>  	char raw_gid[ETH_ALEN];
> 
> -	/* physical port state (only one port per device) */
> -	enum ib_port_state state;
> -
>  	spinlock_t lock;
> 
>  	struct xarray qp_xa;
> diff --git a/drivers/infiniband/sw/siw/siw_cm.c
> b/drivers/infiniband/sw/siw/siw_cm.c
> index 86323918a570..b157bd01e70b 100644
> --- a/drivers/infiniband/sw/siw/siw_cm.c
> +++ b/drivers/infiniband/sw/siw/siw_cm.c
> @@ -1759,6 +1759,7 @@ int siw_create_listen(struct iw_cm_id *id, int
> backlog)
>  {
>  	struct socket *s;
>  	struct siw_cep *cep = NULL;
> +	struct net_device *ndev = NULL;
>  	struct siw_device *sdev = to_siw_dev(id->device);
>  	int addr_family = id->local_addr.ss_family;
>  	int rv = 0;
> @@ -1779,9 +1780,15 @@ int siw_create_listen(struct iw_cm_id *id, int
> backlog)
>  		struct sockaddr_in *laddr = &to_sockaddr_in(id->local_addr);
> 
>  		/* For wildcard addr, limit binding to current device only */
> -		if (ipv4_is_zeronet(laddr->sin_addr.s_addr))
> -			s->sk->sk_bound_dev_if = sdev->netdev->ifindex;
> -
> +		if (ipv4_is_zeronet(laddr->sin_addr.s_addr)) {
> +			ndev = ib_device_get_netdev(id->device, SIW_PORT);
> +			if (ndev) {
> +				s->sk->sk_bound_dev_if = ndev->ifindex;
> +			} else {
> +				rv = -ENODEV;
> +				goto error;
> +			}
> +		}
>  		rv = s->ops->bind(s, (struct sockaddr *)laddr,
>  				  sizeof(struct sockaddr_in));
>  	} else {
> @@ -1797,9 +1804,15 @@ int siw_create_listen(struct iw_cm_id *id, int
> backlog)
>  		}
> 
>  		/* For wildcard addr, limit binding to current device only */
> -		if (ipv6_addr_any(&laddr->sin6_addr))
> -			s->sk->sk_bound_dev_if = sdev->netdev->ifindex;
> -
> +		if (ipv6_addr_any(&laddr->sin6_addr)) {
> +			ndev = ib_device_get_netdev(id->device, SIW_PORT);
> +			if (ndev) {
> +				s->sk->sk_bound_dev_if = ndev->ifindex;
> +			} else {
> +				rv = -ENODEV;
> +				goto error;
> +			}
> +		}
>  		rv = s->ops->bind(s, (struct sockaddr *)laddr,
>  				  sizeof(struct sockaddr_in6));
>  	}
> @@ -1861,6 +1874,9 @@ int siw_create_listen(struct iw_cm_id *id, int
> backlog)
>  	list_add_tail(&cep->listenq, (struct list_head *)id->provider_data);
>  	cep->state = SIW_EPSTATE_LISTENING;
> 
> +	if (ndev)
> +		dev_put(ndev);
> +
>  	siw_dbg(id->device, "Listen at laddr %pISp\n", &id->local_addr);
> 
>  	return 0;
> @@ -1880,6 +1896,9 @@ int siw_create_listen(struct iw_cm_id *id, int
> backlog)
>  	}
>  	sock_release(s);
> 
> +	if (ndev)
> +		dev_put(ndev);
> +
>  	return rv;
>  }
> 
> diff --git a/drivers/infiniband/sw/siw/siw_main.c
> b/drivers/infiniband/sw/siw/siw_main.c
> index 17abef48abcd..14d3103aee6f 100644
> --- a/drivers/infiniband/sw/siw/siw_main.c
> +++ b/drivers/infiniband/sw/siw/siw_main.c
> @@ -287,7 +287,6 @@ static struct siw_device *siw_device_create(struct
> net_device *netdev)
>  		return NULL;
> 
>  	base_dev = &sdev->base_dev;
> -	sdev->netdev = netdev;
> 
>  	if (netdev->addr_len) {
>  		memcpy(sdev->raw_gid, netdev->dev_addr,
> @@ -381,12 +380,10 @@ static int siw_netdev_event(struct notifier_block
> *nb, unsigned long event,
> 
>  	switch (event) {
>  	case NETDEV_UP:
> -		sdev->state = IB_PORT_ACTIVE;
>  		siw_port_event(sdev, 1, IB_EVENT_PORT_ACTIVE);
>  		break;
> 
>  	case NETDEV_DOWN:
> -		sdev->state = IB_PORT_DOWN;
>  		siw_port_event(sdev, 1, IB_EVENT_PORT_ERR);
>  		break;
> 
> @@ -407,12 +404,8 @@ static int siw_netdev_event(struct notifier_block *nb,
> unsigned long event,
>  		siw_port_event(sdev, 1, IB_EVENT_LID_CHANGE);
>  		break;
>  	/*
> -	 * Todo: Below netdev events are currently not handled.
> +	 * All other events are not handled
>  	 */
> -	case NETDEV_CHANGEMTU:
> -	case NETDEV_CHANGE:
> -		break;
> -
>  	default:
>  		break;
>  	}
> @@ -442,12 +435,6 @@ static int siw_newlink(const char *basedev_name,
> struct net_device *netdev)
>  	sdev = siw_device_create(netdev);
>  	if (sdev) {
>  		dev_dbg(&netdev->dev, "siw: new device\n");
> -
> -		if (netif_running(netdev) && netif_carrier_ok(netdev))
> -			sdev->state = IB_PORT_ACTIVE;
> -		else
> -			sdev->state = IB_PORT_DOWN;
> -
>  		ib_mark_name_assigned_by_user(&sdev->base_dev);
>  		rv = siw_device_register(sdev, basedev_name);
>  		if (rv)
> diff --git a/drivers/infiniband/sw/siw/siw_verbs.c
> b/drivers/infiniband/sw/siw/siw_verbs.c
> index 986666c19378..35f3744a3007 100644
> --- a/drivers/infiniband/sw/siw/siw_verbs.c
> +++ b/drivers/infiniband/sw/siw/siw_verbs.c
> @@ -171,21 +171,29 @@ int siw_query_device(struct ib_device *base_dev,
> struct ib_device_attr *attr,
>  int siw_query_port(struct ib_device *base_dev, u32 port,
>  		   struct ib_port_attr *attr)
>  {
> -	struct siw_device *sdev = to_siw_dev(base_dev);
> +	struct net_device *ndev;
>  	int rv;
> 
>  	memset(attr, 0, sizeof(*attr));
> 
>  	rv = ib_get_eth_speed(base_dev, port, &attr->active_speed,
>  			 &attr->active_width);
> +	if (rv)
> +		return rv;
> +
> +	ndev = ib_device_get_netdev(base_dev, SIW_PORT);
> +	if (!ndev)
> +		return -ENODEV;
> +
>  	attr->gid_tbl_len = 1;
>  	attr->max_msg_sz = -1;
> -	attr->max_mtu = ib_mtu_int_to_enum(sdev->netdev->mtu);
> -	attr->active_mtu = ib_mtu_int_to_enum(sdev->netdev->mtu);
> -	attr->phys_state = sdev->state == IB_PORT_ACTIVE ?
> +	attr->max_mtu = ib_mtu_int_to_enum(ndev->max_mtu);
> +	attr->active_mtu = ib_mtu_int_to_enum(READ_ONCE(ndev->mtu));
> +	attr->phys_state = (netif_running(ndev) && netif_carrier_ok(ndev)) ?
>  		IB_PORT_PHYS_STATE_LINK_UP : IB_PORT_PHYS_STATE_DISABLED;
> +	attr->state = attr->phys_state == IB_PORT_PHYS_STATE_LINK_UP ?
> +		IB_PORT_ACTIVE : IB_PORT_DOWN;
>  	attr->port_cap_flags = IB_PORT_CM_SUP | IB_PORT_DEVICE_MGMT_SUP;
> -	attr->state = sdev->state;
>  	/*
>  	 * All zero
>  	 *
> @@ -199,6 +207,7 @@ int siw_query_port(struct ib_device *base_dev, u32
> port,
>  	 * attr->subnet_timeout = 0;
>  	 * attr->init_type_repy = 0;
>  	 */
> +	dev_put(ndev);
>  	return rv;
>  }
> 
> @@ -506,6 +515,7 @@ int siw_query_qp(struct ib_qp *base_qp, struct
> ib_qp_attr *qp_attr,
>  {
>  	struct siw_qp *qp;
>  	struct siw_device *sdev;

The sdev is no longer needed. Sorry for not catching it.
I'll resend v2.


> +	struct net_device *ndev;
> 
>  	if (base_qp && qp_attr && qp_init_attr) {
>  		qp = to_siw_qp(base_qp);
> @@ -513,13 +523,17 @@ int siw_query_qp(struct ib_qp *base_qp, struct
> ib_qp_attr *qp_attr,
>  	} else {
>  		return -EINVAL;
>  	}
> +	ndev = ib_device_get_netdev(base_qp->device, SIW_PORT);
> +	if (!ndev)
> +		return -ENODEV;
> +
>  	qp_attr->qp_state = siw_qp_state_to_ib_qp_state[qp->attrs.state];
>  	qp_attr->cap.max_inline_data = SIW_MAX_INLINE;
>  	qp_attr->cap.max_send_wr = qp->attrs.sq_size;
>  	qp_attr->cap.max_send_sge = qp->attrs.sq_max_sges;
>  	qp_attr->cap.max_recv_wr = qp->attrs.rq_size;
>  	qp_attr->cap.max_recv_sge = qp->attrs.rq_max_sges;
> -	qp_attr->path_mtu = ib_mtu_int_to_enum(sdev->netdev->mtu);
> +	qp_attr->path_mtu = ib_mtu_int_to_enum(READ_ONCE(ndev->mtu));
>  	qp_attr->max_rd_atomic = qp->attrs.irq_size;
>  	qp_attr->max_dest_rd_atomic = qp->attrs.orq_size;
> 
> @@ -534,6 +548,7 @@ int siw_query_qp(struct ib_qp *base_qp, struct
> ib_qp_attr *qp_attr,
> 
>  	qp_init_attr->cap = qp_attr->cap;
> 
> +	dev_put(ndev);
>  	return 0;
>  }
> 
> --
> 2.38.1