Hi Kurt,

It's appreciated to get your suggestions on this.

In short, I would say that this is a simple (or as you said rough) protection in the layer of kernel.
It intentionally to make it configurable only at build time and in the kernel rather configurable in the user space at run/build time as it's just for that kernel layer.

There would be multi layers protection: in the user space, in the kernel running on the application processor, in the ECU or some IO controller, in some hardware settings, etc. This could be part of the whole protection.

Besides, some comments inline with your previous email.
Again, it would be appreciated if some of you experts can share with the details of using queueing discipline to achieve this.

Thanks!


Zhongjie


-----Original Message-----
From: Kurt Van Dijck [mailto:dev.kurt@vandijck-laurijssen.be] 
Sent: Friday, September 01, 2017 3:53 PM
To: Shi, Zhongjie <redacted>
Cc: Justin Delegard <redacted>; Austin Schuh <redacted>; Oliver Hartkopp <socketcan@hartkopp.net>; Marc Kleine-Budde <mkl@pengutronix.de>; linux-can <redacted>
Subject: Re: [PATCH] Add TX limit for SocketCAN.

Hey,

I don't like the approach either.
See my comments below.

Hi Justin,

Sorry for my late response. 
First, I need to clarify that this limit is just for user space programs.
And please let me try to clarify the requirement in more details on our side:

(1) The change will be used for some IVI product.

That's a choice, not a justification for the patch. 

quoted

quoted

Zhongjie: I'm sorry that I didn't mean to make it as justification. I put it here as a fact/context of this change.

(2) In IVI, there are hundreds of use space processes running which has network access which has more risk to be compromised.
(3) And for the IVI system, the TX rate isn't (or shouldn't be) that high as you mentioned. Only possible trigger of TX is from human: the user of IVI.
Please consider #2 and #3, once there are some malicious/buggy program send CAN messages frequently, then it will be harmful to the whole CAN network.

Do you consider your IVI product having this patch as a safe ECU then?

quoted

quoted

Zhongjie: No. It can only part of the whole protection. As I mentioned, we can have protection in multiple layer.

It's a very rough method, since packets are considered good or bad based on the number of recent packets.
That's like protecting your email server by rate-limiting ethernet packets.
It doesn't protect against spam, does it? It's just a rate limiter.

quoted

quoted

Zhongjie: I agree it's rough/simple. To make it to identify the Good/Bad things, there will be more work to be done. I tried add some more complicated statistics in the /proc/net/can/stats and launch some user space watchdog to do the audit. I may share with that sometime later. It's doable that way but much more complicated. With this change, I just want to make it work as simple as possible while fulfill our requirement. The limit to the rate just works in the layer of kernel to do the protection against the worst case of denial of service attack.

And by this logic in my patch, I'm trying to just to prevent the abnormal TX rate from user space on IVI product in the worst case:
either some bug in the user space programs; or some compromised 
program by some hackers with certain zero day attack.

It's up to userspace to decide when a tx rate is abnormal.

quoted

quoted

Zhongjie: Again, I would stress that this is another layer of protection in the kernel. We can have different layers of protection which include the user spaces and the kernel.

For the ECUs on the CAN bus, I'm not sure why is it not possible to set different config for their case.
I mean, e.g. 
(a) On the bus, for IVI, I can use it as 5/s,
(b) For the cases mentioned by Austin, the config could be 10M for their saturation work.
It's just configurable for every nodes on the bus.

I looks like you set up a simple 'firewall' to CAN, with 1 hardcoded rule in kernel code.

quoted

quoted

Zhongjie: Yes :D. I would say implemented in a few line code changes with 1 configurable parameter.

Kind regards,
Kurt