Hi Phillip,
On 16.12.20 05:33, Phillip Schichtel wrote:
Hi everyone!
This is my first post to this mailing list (or any kernel mailing
list), so please tell me if this is the wrong place for this kind
of
topic.
Welcome :-)
You are perfectly right here.
I'm developing a Java binding library to SocketCAN using JNI [1],
where
I try to provide a reasonably "Java-like" yet efficient and safe
API.
Great idea!
Part of this are setters and getters for the SOL_CAN_* socket
options,
which is straight forward for all options except CAN_RAW_FILTER,
since
it is the only option with a dynamically sized value (struct
can_filter*). Setting the value is simple, since all the
information is
available in user space, but when using getsockopt I'm expected to
provide a buffer and a size, but I don't know how many filters
there
are without keeping that state in the library or application,
risking
it going out of sync with the kernel. Is this correct thus far or
am I
missing something? Relevant source on the kernel side is at [2].
On the user space side using getsockopt() I see three ways around
this
issue:
1. Track the amount of filters in user space. I feel like this
might be
problematic if e.g. sockets get shared between threads and
processes.
Other bindings usually take this approach as far as I could tell,
if
they support getting filters at all.
IMO the filters are intended as write-only as it is very common to
set
the filters once at process start and live with them until the
process
terminates.
The getsockopt for CAN_RAW_FILTER was only for completion sake - but
in
fact I did not really think about the expected buffer length in
userspace when reading back a 'bigger' filter list :-/
2. Allocate a buffer large enough that the filters will most likely
all
fit, the optlen will be corrected to the actual size. This is the
approach I currently take (see [3]), but it feels very wrong.
3. Search for the right size by trying increasingly larger buffers
until the buffer is big enough to fit all. This would be kind of an
improvement to 2. for the common case.
Neither of these feel good to me, but maybe that is just me?
No. As we provide the getsockopt() for CAN_RAW_FILTER this way of
'testing out' the filter size is no fun for the programmer.
And using SocketCAN should be fun :-)
On the
kernel side ([2]), I could imagine the option taking a void** for
optval and the kernel allocating a new buffer for the caller and
writing its address to the given pointer and the real length to
optlen,
kind of like this (without knowing the appropriate functions):
case CAN_RAW_FILTER:
lock_sock(sk);
void* filters = NULL;
if (ro->count > 0) {
int fsize = ro->count * sizeof(struct can_filter);
filters = allocate_to_user(fsize);
if (!optval)
err = -EFAULT;
if (copy_to_user(optval, ro->filter, fsize))
err = -EFAULT;
} else {
len = 0;
}
release_sock(sk);
if (!err)
err = put_user(len, optlen);
if (!err)
err = put_user(filters, optval);
return err;
The setsockopt implementation of the option could also be adapted
to
take the same void**.
Alternatively the implementation could always write back the full
size
to optlen instead of the "written size" (put_user(fsize, optlen)
instead of put_user(len, optlen) in code). Since the caller knows
how
big its buffer is, the size necessary would be the more valuable
information.
Did I completely misunderstand something or is this really a
limitation
of the current implementation of this option? And if the latter is
true, are we in the position to change anything about this without
breaking user space?
Yes, you hit the point. We have a limitation in the current
implementation; and no, we must not break user space.
I also haven't really looked into how other protocols handle
dynamically sized option values or if that is even a thing else
where.
Yes. I also had to google and read some kernel code.
When we take a look into the can/raw.c code
https://elixir.bootlin.com/linux/v5.10.1/source/net/can/raw.c#L663
case CAN_RAW_FILTER:
lock_sock(sk);
if (ro->count > 0) {
int fsize = ro->count * sizeof(struct
can_filter);
if (len > fsize)
len = fsize;
if (copy_to_user(optval, ro->filter, len))
At this point we silently truncate the filters to the given length of
the userspace buffer. That's safe but not really good ...
err = -EFAULT;
} else {
len = 0;
}
release_sock(sk);
if (!err)
err = put_user(len, optlen);
return err;
The only interesting code that handles this kind of variable data
vector
read was in net/core/sock.c in sock_getsockopt() for SO_PEERGROUPS:
https://elixir.bootlin.com/linux/v5.10.1/source/net/core/sock.c#L1429
It was introduced in commit 28b5ba2aa0f55:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=28b5ba2aa0f55
"That is, if the provided buffer is too small, ERANGE is returned and
@optlen is updated. Otherwise, the information is copied, @optlen is
set to the actual size, and 0 is returned."
This sounds like an interesting approach.
What do you think about integrating this kind of -ERANGE
functionality
into can/raw.c ?
In fact I never saw someone to use the getsockopt() for
CAN_RAW_FILTER
until now. That's probably the reason why you hit this issue just
now.
IMO introducing the -ERANGE error number does not make the current
situation worse and when a programmer properly checks the return
value
this -ERANGE would lead to some error handling as -EFAULT does today.
So
I would not see that we are breaking user space here, right?
Regards,
Oliver
