POSIX, sockets: Elaborate ioctl/fcntl handling #12002

pfalcon · 2018-12-11T09:06:01Z

I saw some issues in #11438 related to fcntl() handling, but didn't want to procrastinate that PR further, instead intending to look into addressing those issues myself. Turns out that there's even more to it than I initially thought. This is WIP on addressing them, this PR and description will be updated.

@rlubos: In either case, thanks again for embarking on that patch. it turned out to be much more involved than even I initially anticipated, so again, thanks for going for it, and sharing the experience of working with/further developing fdtable support. Elaboration of further issues with it is definitely a common matter.

zephyrbot · 2018-12-11T09:09:34Z

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

codecov-io · 2018-12-11T09:44:55Z

Codecov Report

Merging #12002 into master will increase coverage by 0.01%.
The diff coverage is 22.72%.

@@            Coverage Diff             @@
##           master   #12002      +/-   ##
==========================================
+ Coverage   48.05%   48.06%   +0.01%     
==========================================
  Files         281      283       +2     
  Lines       43414    43425      +11     
  Branches    10404    10406       +2     
==========================================
+ Hits        20862    20872      +10     
  Misses      18403    18403              
- Partials     4149     4150       +1

Impacted Files	Coverage Δ
lib/libc/minimal/include/fcntl.h	`100% <ø> (ø)`
include/misc/fdtable.h	`100% <100%> (ø)`
subsys/net/lib/sockets/sockets.c	`36.36% <21.05%> (+0.9%)`	⬆️
lib/fdtable.c	`29.21% <5%> (-0.12%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 523acef...64d9479. Read the comment docs.

pfalcon · 2018-12-11T11:12:51Z

Some comments: I thought that #11438 kinda tries to funnel all fcntl() command codes via a single ioctl code that PR defined, ZFD_IOCTL_FCNTL. My idea was instead that fcntl() can be made an alias for ioctl(), with command codes for both sharing the same namespace. That's not exactly the right way to fully deal with, but would work as a first approximation.

But when I started to look in the code, I saw that code in #11438 actually mixed 2 approaches. I dispatched fcntl to ioctl as if there were aliases, and yet did that only for ZFD_IOCTL_FCNTL. That generally couldn't work, and yet, as mentioned by @rlubos in
It kinda #11438 (comment) , the litmus test for that stuff, echo_async sample, did. How that was possible is:

        ZFD_IOCTL_CLOSE = 1,
        ZFD_IOCTL_FSYNC,
        ZFD_IOCTL_LSEEK,
        ZFD_IOCTL_FCNTL,

#define F_GETFL 3
#define F_SETFL 4

So, as can be seen, value of F_SETFL coincided with ZFD_IOCTL_FCNTL! So, we actually could set socket to blocking/non-blocking. F_GETFL mapped to LSEEK however, that's not supported for sockets and lead to -1 error, which we then ORed/NANDed with O_NONBLOCK, so overall all worked ;-).

So, in the TDD approach, the first commit here makes that sample fail with the current master by added proper error handling for fcntl().

It then makes fcntl vs ioctl command set disjoint, and makes "fcntl is an alias for ioctl" approach actually work as expected.

Translating that to TLS sockets, I spotted another issue: #11438 (comment) , which required deeper refactoring. Stay tuned for further changes, even though that's going to scope-creep up this PR noticeably.

pfalcon · 2018-12-11T11:14:53Z

@andrewboie: And while I'm trying to avoid adding more noise re: syscalls after the recent "discussions", we still need to further the matter. So, can you confirm whether syscalls are ready to wrap vararg functions like

int ioctl(int fd, unsigned long request, ...);

?

rlubos · 2018-12-11T11:24:12Z

Just to clarify:

I thought that #11438 kinda tries to funnel all fcntl() command codes via a single ioctl code that PR defined, ZFD_IOCTL_FCNTL

That was the intention,

I saw that code in #11438 actually mixed 2 approaches. I dispatched fcntl to ioctl as if there were aliases, and yet did that only for ZFD_IOCTL_FCNTL

and that was the overlook (bug) from my side, the vtable call to ioctl was missing ZFD_IOCTL_FCNTL as a request argument. Nothing more, nothing less.

Nontheless, aliasing should work as well, no objection with that.

pfalcon · 2018-12-11T15:05:31Z

Ok, I believe this now contains all needed stuff.

andyross · 2018-12-11T19:20:12Z

(Oops, realized I dropped this into the PR and not the bit ioctl discussion, will repaste)

Regarding varargs: ioctl() is not really a varargs function. It has varargs linkage to the C side (for historical reasons, and because the third argument is technically optiional), but the actual linux system call takes exactly three arguments (descriptor, request, pointer). The kernel does not walk the stack of the calling process to parse arguments, and it would be insane to try.

On my feelings: I just don't see the value in all the ioctl "rigor". All ioctl does is give you a way to pass a number and a (user) pointer into a driver-supplied callback in the kernel. The numbers (in theory, but NOT PRACTICE) all go into a registry somewhere so they're known to be unique and validatable by the kernel in an automated way. And the pointer allows you to pass all sorts of crazy data structures for the kernel to unpack using a special cross-address-space API (THIS IS NOT A GOOD THING).

Right now, our existing syscall hashing layer gets you that unique number registry for free, without trouble. And our automatic passing of function arguments does 60-80% of what most calls actually need in practice without forcing the handler to read user memory. And we don't need to have a special namespace/registry of these things becuase their names are just C symbols and the linker does that checking for us. Basically: our stuff is better than ioctl already in most ways.

Really, the only feature I see from above that seems to be requested that we don't have is that it would be good to have an automatic checking layer so that function calls can authenticate driver support for a particular call. So if you have two UART devices and only one supports uart_set_stop_bits(), it would be nice for the kernel to fail it automatically, and (maybe) answer the question "does this API work on this device?".

pfalcon · 2018-12-11T19:34:13Z

@andyross

Really, the only feature I see from above that seems to be requested that we don't have

Was this comment posted at the right ticket?

andyross · 2018-12-11T19:35:30Z

(It was not. I saw the note about ioctl() and just assumed this was #12002 where I'd been planning on dumping a bunch of thoughts. Apologies)

Edit: Gah, #11993! Maybe I shouldn't be githubbing today...

andrewboie · 2018-12-11T21:08:10Z

int (*ioctl)(void *obj, unsigned int request, va_list args);

MISRA-C forbids all use of <stdarg.h>, it's a requirement.
AFAIK we're filing exceptions for things like printk(), but some other varargs stuff like k_thread_access_grant() is being removed.

pfalcon · 2018-12-11T21:31:38Z

@andrewboie: I'm puzzled by your answer. Can you just tell if syscalls machinery as it is now supports varargs in syscalls. printk() is a good example indeed, can it be made a syscall? (As is, without userspace-side wrappers.)

If not, the expected syscall name (which is not part of this patch, still on todo) will be sys_vioctl.

pfalcon · 2018-12-11T21:32:16Z

int (*ioctl)(void *obj, unsigned int request, va_list args);

I also accept suggestions of changing the fdtable vmethod name from ->ioctl() to ->vioctl(). (All a-la printf vs vprintf of course.)

andrewboie · 2018-12-11T22:02:44Z

@andrewboie: I'm puzzled by your answer. Can you just tell if syscalls machinery as it is now supports varargs in syscalls.

We do not, and never will.

printk() is a good example indeed, can it be made a syscall? (As is, without userspace-side wrappers.)

No it can't. We'd never want to do that it's a security nightmare, we format the string in user mode and then send the fully formatted string in chunks of 32-bytes at a time.

pfalcon · 2018-12-11T22:25:41Z

Ok, will implement ioctl() on userspace side as a wrapper which takes ... and dispatches to the va_list-taking syscall. So only more reasons to clearly distinguish between (the names) of libc functions and kernel syscalls, as was discussed in the original PR which introduced fdtable.

andrewboie · 2018-12-11T22:34:27Z

va_list-taking syscall

No, don't. You can't send the va_list to the kernel and expect the kernel to do unwinding of the user mode stack.
@andyross wasn't ambiguous:

ioctl() is not really a varargs function. It has varargs linkage to the C side (for historical reasons, and because the third argument is technically optiional), but the actual linux system call takes exactly three arguments (descriptor, request, pointer). The kernel does not walk the stack of the calling process to parse arguments, and it would be insane to try

rlubos

Minor nitpick, otherwise looks good to me.

rlubos · 2018-12-12T08:30:52Z

tests/posix/fs/src/test_fs_file.c

+
+	if (strcmp(test_str + 2, read_buff)) {
+		TC_PRINT("Error - Data read does not match data written\n");
+		TC_PRINT("Data read:\"%s\"\n\n", read_buff);


Double empty line - not sure if it was intended.

@rlubos, you mean in the TC_PRINT output? That's just copy-paste from existing code, which thus does that. If fixed, should be done consistently, and patching test output of this test is completely unrelated to the state description of the patch/PR. (I actually gave in, and patched one case, if you look above in this file - but I verified that with it the output is guaranteedly looks better, and without it - worse).

It only drew my attention beacuse you actually fix similar case a few lines above.
But really, it's such a big nit that I don't see a reason to elaborate/argue on that.

you actually fix similar case a few lines above.

Just one of a dozen.

But really, it's such a big nit that I don't see a reason to elaborate/argue on that.

Yeah, tests are tests - it's good that someone writes them and elaborates at all ;-).

pfalcon · 2018-12-12T12:03:01Z

@rlubos: So, anything left to get +1 from you?

rlubos

LGTM 👍

subsys/net/lib/sockets/sockets.c

samples/net/sockets/echo_async/src/socket_echo.c

Check result of fcntl() to catch any regressions in fcntl() handling in Zephyr. To facilitate this, also merge block() and nonblock() functions into single setblocking(). Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

fcntl operations are implemented using ioctl vmethod. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

Check that skipping a few initial bytes of file gives the expected result. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

As extend fdtable usage to more cases, there regularly arises a need to forward ioctl/fcntl arguments to another ioctl vmethod, which is complicated because it defined as taking variadic arguments. The only portable solution is to convert variadic arguments to va_list at the first point of entry from client code, and then pass va_list around. To facilitate calling ioctl with variadic arguments from system code, z_fdtable_call_ioctl() helper function is added. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

ioctl() just dispatches to the corresponding vmethod of an fd. fcntl() handles fdtable-level operations (so far doesn't handle actually, returning "not implemented" error), and forwards fd-specific operations to ioctl vmethod just the same (i.e. ioctl and fcntl operations share the same namespace, but otherwise disjoint). Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

s/DLTS/DTLS. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

pfalcon · 2018-12-12T13:09:01Z

Pushed codestyle fixes suggested by @jukkar. Also, a minor typo fix in sockets_tls.c.

pfalcon · 2018-12-14T07:23:08Z

Ping @jukkar, @tbursztyka .

jukkar · 2018-12-14T09:08:46Z

Ping @andrewboie are you ok with this change, you had comments about this earlier?

pfalcon · 2018-12-14T09:11:54Z

@jukkar:

Ping @andrewboie are you ok with this change, you had comments about this earlier?

The question I asked to @andrewboie and the discussion which ensued was about planning next changes beyond this patch.

jukkar · 2018-12-14T11:21:12Z

was about planning next changes beyond this patch.

Ok, it was not obvious when looking the discussion. Lets merge this then.

pfalcon requested a review from rlubos December 11, 2018 09:06

pfalcon requested review from andrewboie, andyross, jukkar and tbursztyka as code owners December 11, 2018 09:06

pfalcon mentioned this pull request Dec 11, 2018

sockets: tls: Make secure sockets support posix APIs #11438

Merged

pfalcon force-pushed the fdtable-ioctl-fcntl branch from 218d2a9 to 7cd2c54 Compare December 11, 2018 09:07

jukkar added the area: Networking label Dec 11, 2018

pfalcon force-pushed the fdtable-ioctl-fcntl branch from 8597363 to 7ee9c36 Compare December 11, 2018 14:59

pfalcon requested review from nniranjhana and ramakrishnapallala as code owners December 11, 2018 14:59

pfalcon changed the title ~~[WIP] sockets: Rework/clarify fcntl handling~~ POSIX, sockets: Elaborate ioctl/fcntl handling Dec 11, 2018

pfalcon force-pushed the fdtable-ioctl-fcntl branch from 7ee9c36 to 5c82628 Compare December 11, 2018 15:06

pfalcon mentioned this pull request Dec 11, 2018

lib: fdtable: fsync() should not result in fd closure. #12008

Closed

pfalcon force-pushed the fdtable-ioctl-fcntl branch from 5c82628 to 915bedd Compare December 11, 2018 20:05

rlubos reviewed Dec 12, 2018

View reviewed changes

rlubos approved these changes Dec 12, 2018

View reviewed changes

jukkar reviewed Dec 12, 2018

View reviewed changes

subsys/net/lib/sockets/sockets.c Show resolved Hide resolved

subsys/net/lib/sockets/sockets.c Show resolved Hide resolved

samples/net/sockets/echo_async/src/socket_echo.c Show resolved Hide resolved

pfalcon added 6 commits December 12, 2018 16:08

net: sockets: Rework fcntl() dispatching to the underlying fd object

3b9e072

fcntl operations are implemented using ioctl vmethod. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

tests: posix: test_fs_file: Add testcase with non-zero lseek()

c3e89ef

Check that skipping a few initial bytes of file gives the expected result. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

net: sockets_tls: Typo fix in comment

64d9479

s/DLTS/DTLS. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

pfalcon force-pushed the fdtable-ioctl-fcntl branch from 915bedd to 64d9479 Compare December 12, 2018 13:08

tbursztyka approved these changes Dec 14, 2018

View reviewed changes

jukkar approved these changes Dec 14, 2018

View reviewed changes

jukkar merged commit 8a65f68 into zephyrproject-rtos:master Dec 14, 2018

jukkar mentioned this pull request Feb 9, 2022

RFC: networking: Make fcntl for sockets work like Linux #42612

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

POSIX, sockets: Elaborate ioctl/fcntl handling #12002

POSIX, sockets: Elaborate ioctl/fcntl handling #12002

pfalcon commented Dec 11, 2018

zephyrbot commented Dec 11, 2018 •

edited

Loading

codecov-io commented Dec 11, 2018 •

edited

Loading

pfalcon commented Dec 11, 2018

pfalcon commented Dec 11, 2018

rlubos commented Dec 11, 2018

pfalcon commented Dec 11, 2018

andyross commented Dec 11, 2018 •

edited

Loading

pfalcon commented Dec 11, 2018

andyross commented Dec 11, 2018 •

edited

Loading

andrewboie commented Dec 11, 2018

pfalcon commented Dec 11, 2018

pfalcon commented Dec 11, 2018

andrewboie commented Dec 11, 2018

pfalcon commented Dec 11, 2018

andrewboie commented Dec 11, 2018 •

edited

Loading

rlubos left a comment

rlubos Dec 12, 2018

pfalcon Dec 12, 2018

rlubos Dec 12, 2018

pfalcon Dec 12, 2018

pfalcon commented Dec 12, 2018

rlubos left a comment

pfalcon commented Dec 12, 2018

pfalcon commented Dec 14, 2018

jukkar commented Dec 14, 2018

pfalcon commented Dec 14, 2018

jukkar commented Dec 14, 2018

POSIX, sockets: Elaborate ioctl/fcntl handling #12002

POSIX, sockets: Elaborate ioctl/fcntl handling #12002

Conversation

pfalcon commented Dec 11, 2018

zephyrbot commented Dec 11, 2018 • edited Loading

codecov-io commented Dec 11, 2018 • edited Loading

Codecov Report

pfalcon commented Dec 11, 2018

pfalcon commented Dec 11, 2018

rlubos commented Dec 11, 2018

pfalcon commented Dec 11, 2018

andyross commented Dec 11, 2018 • edited Loading

pfalcon commented Dec 11, 2018

andyross commented Dec 11, 2018 • edited Loading

andrewboie commented Dec 11, 2018

pfalcon commented Dec 11, 2018

pfalcon commented Dec 11, 2018

andrewboie commented Dec 11, 2018

pfalcon commented Dec 11, 2018

andrewboie commented Dec 11, 2018 • edited Loading

rlubos left a comment

Choose a reason for hiding this comment

rlubos Dec 12, 2018

Choose a reason for hiding this comment

pfalcon Dec 12, 2018

Choose a reason for hiding this comment

rlubos Dec 12, 2018

Choose a reason for hiding this comment

pfalcon Dec 12, 2018

Choose a reason for hiding this comment

pfalcon commented Dec 12, 2018

rlubos left a comment

Choose a reason for hiding this comment

pfalcon commented Dec 12, 2018

pfalcon commented Dec 14, 2018

jukkar commented Dec 14, 2018

pfalcon commented Dec 14, 2018

jukkar commented Dec 14, 2018

zephyrbot commented Dec 11, 2018 •

edited

Loading

codecov-io commented Dec 11, 2018 •

edited

Loading

andyross commented Dec 11, 2018 •

edited

Loading

andyross commented Dec 11, 2018 •

edited

Loading

andrewboie commented Dec 11, 2018 •

edited

Loading