Hi Vincent,
apologies for the late reply.
On 07-07-2024 17:23, Vincent Mailhol wrote:
Hi Matthias,
On Fri. 5 Jul. 2024 at 02:47, Matthias Unterrainer
<matthias.unterrainer@xxxxxxxxxxxxxxxxxxxx> wrote:
Hi Linux-CAN Community,
my name is Matthias and I recently developed a kernel module during my Bachelor's thesis that allows for transferring CAN frames over IP networks, similar to userland tools like socketcand [0] or cannelloni [1].
I wrote the thesis at dissecto GmbH [2], a german Startup that specializes in security diagnostics and analytics for embedded systems, primarily within the automotive industry.
The idea behind the project is that dissecto has developed a hardware device that can be connected to a CAN bus and acts as an ethernet gateway to the bus. It is capable of capturing the CAN traffic along with the corresponding timestamps and send this data via UDP or it can receive CAN frames via UDP as well and pass them on to the CAN bus.
This allows for remote interaction with a CAN bus, as well as an accurate analyses of CAN traffic, as packets contain precise time stamps.
An architectural design decision was to develop it as kernel module because of lower latencies and high throughput.
Question: did you consider Packet MMAP?
https://docs.kernel.org/networking/packet_mmap.html
Most of the overhead comes from the syscall context switch between the
user and kernel land and Packet MMAP is exactly designed to bypass
this. Actually, a few months ago, I started to rewrite the can-utils's
candump to use Packet MMAP, but I never finished it.
No, at the time I did not consider Packet MMAP. From my understanding
Packet MMAP is used for userspace applications. But the module has to
modify the timestamps of the CAN frames as they appear on the interface,
which, as far as I know, is not possible from userspace.
Please correct me if I am mistaken, but because of that I do not think
Packet MMAP or any userspace application for that matter could actually
be used here.
For example, my measurements show that the average time it takes a CAN frame to get processed by the module is just about 1/4 of the time it takes applications like socketcand or cannelloni.
We have published the module on GitHub [3], and would appreciate your feedback and thoughts.
If anyone is interested in this functionality for the same or similar use cases, please don't hesitate to contact us.
Best regards
Matthias Unterrainer
I "scrolled" through the code. Here are two quick feedbacks.
Thanks for the feedback. I already partly implemented your feedback and
the rest will come soon.
In struct can2eth_pkthdr:
struct can2eth_pkthdr {
u32 magic;
u32 tv_sec;
u32 tv_nsec;
u16 seqno;
u16 size;
};
If I understand correctly, these are big endian. In That case, use the
__be16 and __be32 types (c.f.
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/types.h#L37).
This will allow use to use sparse to check that all conversion are
done correctly:
https://www.kernel.org/doc/html/latest/dev-tools/sparse.html
Also, you added the description of each of struct can2eth_pkthdr's
fields in the README.md. It is preferable to put it directly together
with the code, using the kernel-doc style:
https://docs.kernel.org/doc-guide/kernel-doc.html
In ctem_setup_communications():
int ctem_setup_communications(struct ctem_comm_handler *handler, int src_port,
char **addrs, int num_addrs)
{
struct sockaddr_in *addr;
u32 ip;
u16 port = 0;
int ret;
int i;
handler->dest_addrs = kmalloc(sizeof(struct sockaddr *), GFP_KERNEL);
if (!handler->dest_addrs) {
pr_err("Could not allocate destination addresses\n");
return -ENOMEM;
}
handler->last_seqno = 0;
handler->num_addrs = 0;
/* parse input ip addresses */
if (!num_addrs) {
addr = kmalloc(sizeof(struct sockaddr_in), GFP_KERNEL);
if (!addr) {
pr_err("Could not allocate ip addr\n");
return -ENOMEM;
}
You are doing a memory allocation here...
addr->sin_family = PF_INET;
addr->sin_port = htons(port);
addr->sin_addr.s_addr = INADDR_ANY;
handler->dest_addrs[0] = (struct sockaddr *)addr;
handler->num_addrs = 1;
} else {
for (i = 0; i < num_addrs; i++) {
ret = parse_ip_port(ip_addrs[i], &ip, &port);
if (ret) {
pr_err("Invalid IP/port format: %s\n",
ip_addrs[i]);
} else {
pr_debug("Parsed IP: %pI4, Port: %u\n", &ip,
port);
addr = kmalloc(sizeof(struct sockaddr_in),
GFP_KERNEL);
if (!addr) {
pr_err("Could not allocate ip addr\n");
return -ENOMEM;
}
... and here ...
addr->sin_family = PF_INET;
addr->sin_port = htons(port);
addr->sin_addr.s_addr = ip;
handler->dest_addrs[handler->num_addrs] =
(struct sockaddr *)addr;
handler->num_addrs++;
}
}
}
ret = ctem_setup_udp(handler, src_port);
if (ret)
return ret;
ret = msgbuilder_init(handler);
if (ret)
return ret;
... but here you directly return on those errors without freeing the
previously allocated memory. Is this OK?
Overall, it will probably be simpler to move your
/* parse input ip addresses */
if/else block in a separate helper function.
handler->reception_thread = kthread_create(
ctem_reception_thread, handler, "ctem_reception_thread");
if (IS_ERR(handler->reception_thread)) {
pr_err("%s: Error creating reception thread\n", MODULE_NAME);
return PTR_ERR(handler->reception_thread);
}
return 0;
}
[0] https://github.com/linux-can/socketcand
[1] https://github.com/mguentner/cannelloni
[2] https://dissec.to
[3] https://github.com/dissecto-GmbH/can2eth-kernel-module
Yours sincerely,
Vincent Mailhol
Best regards,
Matthias Unterrainer
