Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 11 May 2022 14:53:43 +0200
From:   Oleksij Rempel <o.rempel@pengutronix.de>
To:     Oliver Hartkopp <socketcan@hartkopp.net>
Cc:     Wolfgang Grandegger <wg@grandegger.com>,
        Marc Kleine-Budde <mkl@pengutronix.de>,
        Devid Antonio Filoni <devid.filoni@egluetechnologies.com>,
        kernel@pengutronix.de, linux-can@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        David Jander <david@protonic.nl>
Subject: Re: [PATCH 1/1] can: skb: add and set local_origin flag
Message-ID: <20220511125343.GA24267@pengutronix.de>
References: <20220511121913.2696181-1-o.rempel@pengutronix.de>
 <b631b022-72d5-9160-fd13-f33c80dbbe59@hartkopp.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <b631b022-72d5-9160-fd13-f33c80dbbe59@hartkopp.net>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Wed, May 11, 2022 at 02:38:32PM +0200, Oliver Hartkopp wrote:
> Hi Oleksij,
> 
> On 5/11/22 14:19, Oleksij Rempel wrote:
> > Add new can_skb_priv::local_origin flag to be able detect egress
> > packages even if they was sent directly from kernel and not assigned to
> > some socket.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > Cc: Devid Antonio Filoni <devid.filoni@egluetechnologies.com>
> > ---
> >   drivers/net/can/dev/skb.c | 3 +++
> >   include/linux/can/skb.h   | 1 +
> >   net/can/raw.c             | 2 +-
> >   3 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> > index 61660248c69e..3e2357fb387e 100644
> > --- a/drivers/net/can/dev/skb.c
> > +++ b/drivers/net/can/dev/skb.c
> > @@ -63,6 +63,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
> >   		/* save frame_len to reuse it when transmission is completed */
> >   		can_skb_prv(skb)->frame_len = frame_len;
> > +		can_skb_prv(skb)->local_origin = true;
> >   		skb_tx_timestamp(skb);
> > @@ -200,6 +201,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)->local_origin = false;
> >   	*cf = skb_put_zero(skb, sizeof(struct can_frame));
> > @@ -231,6 +233,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)->local_origin = false;
> 
> IMO this patch does not work as intended.
> 
> You probably need to revisit every place where can_skb_reserve() is used,
> e.g. in raw_sendmsg().
> 
> E.g. to make it work for virtual CAN and vxcan interfaces.

ok, i'll take a look on it.

> 
> I'm a bit unsure why we should not stick with the simple skb->sk handling?

In case of J1939 we have kernel generate control frames not associated with any
socket. For example transfer abort messages because no receive socket
was detected. Or there are multiple receive sockets and attaching to one
of it make no sense.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |