linux-kernel - Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and can_skb_headroom

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMZ6RqKZMHXB7rQ70GrXcVE7x7kytAAGfE+MOpSgWgWgp0gD2g@mail.gmail.com>
Date:   Tue, 17 May 2022 10:50:16 +0900
From:   Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To:     Oliver Hartkopp <socketcan@...tkopp.net>
Cc:     Marc Kleine-Budde <mkl@...gutronix.de>, linux-can@...r.kernel.org,
        linux-kernel@...r.kernel.org, Max Staudt <max@...as.org>,
        netdev@...r.kernel.org
Subject: Re: [PATCH v3 3/4] can: skb:: move can_dropped_invalid_skb and
 can_skb_headroom_valid to skb.c

Hi Oliver,

On Mon. 16 May 2022 at 04:17, Oliver Hartkopp <socketcan@...tkopp.net> wrote:
> Hi Vincent,
>
> On 14.05.22 16:16, Vincent Mailhol wrote:
> > The functions can_dropped_invalid_skb() and can_skb_headroom_valid()
> > grew a lot over the years to a point which it does not make much sense
> > to have them defined as static inline in header files. Move those two
> > functions to the .c counterpart of skb.h.
> >
> > can_skb_headroom_valid() only caller being can_dropped_invalid_skb(),
> > the declaration is removed from the header. Only
> > can_dropped_invalid_skb() gets its symbol exported.
>
> I can see your point but the need for can-dev was always given for
> hardware specific stuff like bitrates, TDC, transceivers, whatever.

I also see your point :)
Actually, I raised the exact same idea in a previous message:
https://lore.kernel.org/linux-can/CAMZ6RqLU-Wg0Cau3cM=QsU-t-7Lyzmo1nJ_VAA4Mbw3u0jnNtw@mail.gmail.com/

But you were not in CC and it seems that there is a lot of congestion
recently on the mailing list so I wouldn’t be surprised if you tell me
you did not receive it.

> As there would be more things in slcan (e.g. dev_alloc_skb() could be
> unified with alloc_can_skb())

And also the can_{put,get}_echo_skb() I guess.

> would it probably make sense to
> introduce a new can-skb module that could be used by all CAN
> virtual/software interfaces?
>
> Or some other split-up ... any idea?

My concern is: what would be the merrit? If we do not split, the users
of slcan and v(x)can would have to load the can-dev module which will
be slightly bloated for their use, but is this really an issue? I do
not see how this can become a performance bottleneck, so what is the
problem?
I could also argue that most of the devices do not depend on
rx-offload.o. So should we also split this one out of can-dev on the
same basis and add another module dependency?
The benefit (not having to load a bloated module for three drivers)
does not outweigh the added complexity: all hardware modules will have
one additional modprobe dependency on the tiny can-skb module.

But as said above, I am not fully opposed to the split, I am just
strongly divided. If we go for the split, creating a can-skb module is
the natural and only option I see.
If the above argument does not convince you, I will send a v3 with that split.

Yours sincerely,
Vincent Mailhol