Re: [PATCH v2 1/2] can: fix multiple delivery of a single CAN frame for overlapping CAN filters

["Ahmed S. Darwish" <darwish.07@gmail.com>]

Hi!

On Wed, Mar 18, 2015 at 07:10:34AM +0100, Oliver Hartkopp wrote:
> The CAN_RAW socket can set multiple CAN identifier specific filters that lead
> to multiple filters in the af_can.c filter processing. These filters are
> indenpendent from each other which leads to logical OR'ed filters when applied.
> 
> This patch makes sure that every CAN frame which is filtered for a specific
> socket is only delivered once to the user space. This is independent from the
> number of matching CAN filters of this socket.
> 
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
> 
> v2: move simple check to be processed first
> 
>  net/can/raw.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/net/can/raw.c b/net/can/raw.c
> index 00c13ef..0a6abf1 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -86,6 +86,8 @@ struct raw_sock {
>  	struct can_filter dfilter; /* default/single filter */
>  	struct can_filter *filter; /* pointer to filter(s) */
>  	can_err_mask_t err_mask;
> +	struct sk_buff *uniq_skb;
> +	ktime_t uniq_tstamp;
>  };
>  

My knowledge of the networking internals are really shallow,
but the `uniq_tstamp' field above feels a little bit redundant?

As seen below:

>  /*
> @@ -123,6 +125,15 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
>  	if (!ro->fd_frames && oskb->len != CAN_MTU)
>  		return;
>  
> +	/* eliminate multiple filter matches for the same skb */
> +	if (ro->uniq_skb == oskb &&
> +	    ktime_equal(ro->uniq_tstamp, oskb->tstamp)){
> +		return;
> +	} else {
> +		ro->uniq_skb = oskb;
> +		ro->uniq_tstamp = oskb->tstamp;
> +	}
> +

The two newly-introduced fields are always set together, and they
are not read anywhere beyond the if check above.

If so, isn't the `uniq_skb' field, on its own, quite sufficient?

>  	/* clone the given skb to be able to enqueue it into the rcv queue */
>  	skb = skb_clone(oskb, GFP_ATOMIC);
>  	if (!skb)
> @@ -296,6 +307,8 @@ static int raw_init(struct sock *sk)
>  	ro->loopback         = 1;
>  	ro->recv_own_msgs    = 0;
>  	ro->fd_frames        = 0;
> +	ro->uniq_skb         = NULL;
> +	ro->uniq_tstamp      = ktime_set(0, 0);
>  
>  	/* set notifier */
>  	ro->notifier.notifier_call = raw_notifier;
>

Thanks,
Darwish