On 10.05.2021 20:20:39, Marc Kleine-Budde wrote:
> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index 61660248c69e..9644c0d85bb6 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
> @@ -200,6 +200,7 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
> can_skb_reserve(skb);
> can_skb_prv(skb)->ifindex = dev->ifindex;
> can_skb_prv(skb)->skbcnt = 0;
> + can_skb_prv(skb)->flags = 0;
>
> *cf = skb_put_zero(skb, sizeof(struct can_frame));
>
> @@ -231,6 +232,7 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
> can_skb_reserve(skb);
> can_skb_prv(skb)->ifindex = dev->ifindex;
> can_skb_prv(skb)->skbcnt = 0;
> + can_skb_prv(skb)->flags = 0;
>
> *cfd = skb_put_zero(skb, sizeof(struct canfd_frame));
>
> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> index d311bc369a39..24a3e682b4a2 100644
> --- a/include/linux/can/skb.h
> +++ b/include/linux/can/skb.h
> @@ -51,6 +51,7 @@ struct can_skb_priv {
> int ifindex;
> int skbcnt;
> unsigned int frame_len;
> + unsigned int flags;
> struct can_frame cf[];
> };
>
> @@ -71,9 +72,16 @@ static inline void can_skb_set_owner(struct sk_buff *skb, struct sock *sk)
> * after the last TX skb has been freed). So only increase
> * socket refcount if the refcount is > 0.
> */
> - if (sk && refcount_inc_not_zero(&sk->sk_refcnt)) {
^^^^^^^^^^^^^^^^^^^^^
> - skb->destructor = sock_efree;
> - skb->sk = sk;
This is the problem:
It was introduced by Oleksij and me in e940e0895a82 ("can: skb:
can_skb_set_owner(): fix ref counting if socket was closed before
setting skb ownership").
In e940e0895a82 we fix the ref counting on the socket problem, as
described in the comment of that patch:
| static inline void can_skb_set_owner(struct sk_buff *skb, struct sock *sk)
| {
| - if (sk) {
| - sock_hold(sk);
| + /* If the socket has already been closed by user space, the
| + * refcount may already be 0 (and the socket will be freed
| + * after the last TX skb has been freed). So only increase
| + * socket refcount if the refcount is > 0.
| + */
| + if (sk && refcount_inc_not_zero(&sk->sk_refcnt)) {
| skb->destructor = sock_efree;
| skb->sk = sk;
| }
We missed the fact that skb->sk is used in raw_rcv() to determine if the
CAN frame was received from outside or it was the echo of a TX skb. As a
workaround/fix I introduced a new variable "flags" to the struct
can_skb_priv and added MSG_DONTROUTE directly there, if a socket was
associated with the skb. I haven't tested what that means for vxcan
devices in network name spaces.
> + if (sk) {
> + struct can_skb_priv *skb_priv;
> +
> + skb_priv = can_skb_prv(skb);
> + skb_priv->flags = MSG_DONTROUTE;
> +
> + if (refcount_inc_not_zero(&sk->sk_refcnt)) {
> + skb->destructor = sock_efree;
> + skb->sk = sk;
> + }
> }
> }
>
> diff --git a/net/can/raw.c b/net/can/raw.c
> index 139d9471ddcf..9bedd0672fae 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -169,7 +169,7 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
> /* add CAN specific message flags for raw_recvmsg() */
> pflags = raw_flags(skb);
> *pflags = 0;
> - if (oskb->sk)
> + if (can_skb_prv(oskb)->flags == MSG_DONTROUTE)
> *pflags |= MSG_DONTROUTE;
> if (oskb->sk == sk)
> *pflags |= MSG_CONFIRM;
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
