From 1cd0ae814a0e4d91b32e30d345c98c52118804e8 Mon Sep 17 00:00:00 2001 From: Paul Sokolovsky Date: Tue, 19 May 2020 12:47:29 +0300 Subject: [PATCH 1/2] lib: os: fdtable: Add syscalls for POSIX read/write/close operations Fdtable is a kernel-level structure, so functions accessing it, like read(), write(), close(), should be syscalls. This conversion however emphasizes a difference between "syscall" and "C standard library function". C stdlib functions should be represented by just a standard linkage symbol, and can be used even without prototype. Syscall is however a special code sequence, which may require special attributes and thus a prototype for usage. To deal with these distinction, we define a convention that "sys_name" is a syscall, while "name" is a normal C function (implemented as a wrapper around syscall). (And we also need to define "_name" to interface with Newlib (actually, just "_write".)) Fixes: #25407 Signed-off-by: Paul Sokolovsky --- include/posix/unistd.h | 14 +++++++++- lib/os/fdtable.c | 61 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/include/posix/unistd.h b/include/posix/unistd.h index e9c955c5bf0d1..3765ad4381ae3 100644 --- a/include/posix/unistd.h +++ b/include/posix/unistd.h @@ -22,7 +22,15 @@ extern "C" { #endif #ifdef CONFIG_POSIX_API -/* File related operations */ +/* File related operations. Convention: "sys_name" is a syscall (needs + * prototype in this file for usage). "name" is a normal userspace + * function (implemented as a wrapper for syscall), usable even + * without prototype, per classical C handling. This distinction + * is however implemented on demand, based on the actual usecases seen. + */ +__syscall int sys_close(int file); +__syscall ssize_t sys_write(int file, const void *buffer, size_t count); +__syscall ssize_t sys_read(int file, void *buffer, size_t count); extern int close(int file); extern ssize_t write(int file, const void *buffer, size_t count); extern ssize_t read(int file, void *buffer, size_t count); @@ -50,4 +58,8 @@ int usleep(useconds_t useconds); } #endif +#ifndef CONFIG_ARCH_POSIX +#include +#endif /* CONFIG_ARCH_POSIX */ + #endif /* ZEPHYR_INCLUDE_POSIX_UNISTD_H_ */ diff --git a/lib/os/fdtable.c b/lib/os/fdtable.c index 53dcbecf15379..4efbcb017221e 100644 --- a/lib/os/fdtable.c +++ b/lib/os/fdtable.c @@ -18,6 +18,9 @@ #include #include #include +#ifdef CONFIG_POSIX_API +#include +#endif #include struct fd_entry { @@ -167,7 +170,7 @@ int z_alloc_fd(void *obj, const struct fd_op_vtable *vtable) #ifdef CONFIG_POSIX_API -ssize_t read(int fd, void *buf, size_t sz) +ssize_t z_impl_sys_read(int fd, void *buf, size_t sz) { if (_check_fd(fd) < 0) { return -1; @@ -175,9 +178,30 @@ ssize_t read(int fd, void *buf, size_t sz) return fdtable[fd].vtable->read(fdtable[fd].obj, buf, sz); } + +#ifdef CONFIG_USERSPACE +ssize_t z_vrfy_sys_read(int fd, void *buf, size_t sz) +{ + if (Z_SYSCALL_MEMORY_WRITE(buf, sz)) { + errno = EFAULT; + return -1; + } + + return z_impl_sys_read(fd, buf, sz); +} +#include +#endif /* CONFIG_USERSPACE */ + +/* Normal C function wrapping a corresponding syscall. Required to ensure + * classic C linkage. + */ +ssize_t read(int fd, void *buf, size_t sz) +{ + return sys_read(fd, buf, sz); +} FUNC_ALIAS(read, _read, ssize_t); -ssize_t write(int fd, const void *buf, size_t sz) +ssize_t z_impl_sys_write(int fd, const void *buf, size_t sz) { if (_check_fd(fd) < 0) { return -1; @@ -185,9 +209,27 @@ ssize_t write(int fd, const void *buf, size_t sz) return fdtable[fd].vtable->write(fdtable[fd].obj, buf, sz); } + +#ifdef CONFIG_USERSPACE +ssize_t z_vrfy_sys_write(int fd, const void *buf, size_t sz) +{ + Z_OOPS(Z_SYSCALL_MEMORY_READ(buf, sz)); + + return z_impl_sys_write(fd, buf, sz); +} +#include +#endif /* CONFIG_USERSPACE */ + +/* Normal C function wrapping a corresponding syscall. Required to ensure + * classic C linkage. + */ +ssize_t write(int fd, const void *buf, size_t sz) +{ + return sys_write(fd, buf, sz); +} FUNC_ALIAS(write, _write, ssize_t); -int close(int fd) +int z_impl_sys_close(int fd) { int res; @@ -200,6 +242,19 @@ int close(int fd) return res; } + +#ifdef CONFIG_USERSPACE +ssize_t z_vrfy_sys_close(int fd) +{ + return z_impl_sys_close(fd); +} +#include +#endif /* CONFIG_USERSPACE */ + +int close(int fd) +{ + return sys_close(fd); +} FUNC_ALIAS(close, _close, int); int fsync(int fd) From e089e263fd5708911ceb57065dbf075773fdca5b Mon Sep 17 00:00:00 2001 From: Paul Sokolovsky Date: Thu, 11 Jun 2020 13:28:12 +0300 Subject: [PATCH 2/2] tests: socket: tcp_rw: Test for using read()/write() on socket fd With POSIX subsystem enabled (CONFIG_POSIX_API=y), it's possible to use generic POSIX read()/write() calls on a socket file descriptor. This test verifies their operaration on this case. Signed-off-by: Paul Sokolovsky --- tests/net/socket/socket_helpers.h | 5 + tests/net/socket/tcp_rw/CMakeLists.txt | 9 ++ tests/net/socket/tcp_rw/prj.conf | 35 +++++ tests/net/socket/tcp_rw/src/main.c | 208 +++++++++++++++++++++++++ tests/net/socket/tcp_rw/testcase.yaml | 9 ++ 5 files changed, 266 insertions(+) create mode 100644 tests/net/socket/tcp_rw/CMakeLists.txt create mode 100644 tests/net/socket/tcp_rw/prj.conf create mode 100644 tests/net/socket/tcp_rw/src/main.c create mode 100644 tests/net/socket/tcp_rw/testcase.yaml diff --git a/tests/net/socket/socket_helpers.h b/tests/net/socket/socket_helpers.h index 69bb20c14b02a..e8bea92e3321b 100644 --- a/tests/net/socket/socket_helpers.h +++ b/tests/net/socket/socket_helpers.h @@ -6,7 +6,12 @@ #include +#ifdef CONFIG_POSIX_API +#include +#include +#else #include +#endif #define clear_buf(buf) memset(buf, 0, sizeof(buf)) diff --git a/tests/net/socket/tcp_rw/CMakeLists.txt b/tests/net/socket/tcp_rw/CMakeLists.txt new file mode 100644 index 0000000000000..6e172424cdf2c --- /dev/null +++ b/tests/net/socket/tcp_rw/CMakeLists.txt @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: Apache-2.0 + +cmake_minimum_required(VERSION 3.13.1) +find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE}) +project(socket_tcp_rw) + +target_include_directories(app PRIVATE ${ZEPHYR_BASE}/subsys/net/ip) +FILE(GLOB app_sources src/*.c) +target_sources(app PRIVATE ${app_sources}) diff --git a/tests/net/socket/tcp_rw/prj.conf b/tests/net/socket/tcp_rw/prj.conf new file mode 100644 index 0000000000000..a6faea9d659a1 --- /dev/null +++ b/tests/net/socket/tcp_rw/prj.conf @@ -0,0 +1,35 @@ +# Setup for self-contained net testing without requiring a SLIP driver +CONFIG_NET_TEST=y + +# General config +CONFIG_NEWLIB_LIBC=y +CONFIG_POSIX_API=y + +# Networking config +CONFIG_NETWORKING=y +CONFIG_NET_IPV4=y +CONFIG_NET_IPV6=y +CONFIG_NET_TCP=y +CONFIG_NET_SOCKETS=y +CONFIG_NET_SOCKETS_POSIX_NAMES=y +CONFIG_POSIX_MAX_FDS=20 + +# Network driver config +CONFIG_NET_LOOPBACK=y +CONFIG_TEST_RANDOM_GENERATOR=y + +# Network address config +CONFIG_NET_CONFIG_SETTINGS=y +CONFIG_NET_CONFIG_NEED_IPV4=y +CONFIG_NET_CONFIG_NEED_IPV6=y +CONFIG_NET_CONFIG_MY_IPV4_ADDR="192.0.2.1" +CONFIG_NET_CONFIG_MY_IPV6_ADDR="2001:db8::1" + +CONFIG_MAIN_STACK_SIZE=2048 +CONFIG_TEST_USERSPACE=y + +# The test requires lot of bufs +CONFIG_NET_PKT_TX_COUNT=24 + +CONFIG_ZTEST=y +CONFIG_ZTEST_STACKSIZE=2048 diff --git a/tests/net/socket/tcp_rw/src/main.c b/tests/net/socket/tcp_rw/src/main.c new file mode 100644 index 0000000000000..da414524aca80 --- /dev/null +++ b/tests/net/socket/tcp_rw/src/main.c @@ -0,0 +1,208 @@ +/* + * Copyright (c) 2020 Linaro Limited + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#include +LOG_MODULE_REGISTER(net_test, CONFIG_NET_SOCKETS_LOG_LEVEL); + +#include +#include +#include + +#ifdef CONFIG_POSIX_API +#include +#endif + +#include "../../socket_helpers.h" + +#define TEST_STR_SMALL "test" + +#define ANY_PORT 0 +#define SERVER_PORT 4242 + +#define MAX_CONNS 5 + +#define TCP_TEARDOWN_TIMEOUT K_SECONDS(1) + +static void test_bind(int sock, struct sockaddr *addr, socklen_t addrlen) +{ + zassert_equal(bind(sock, addr, addrlen), + 0, + "bind failed"); +} + +static void test_listen(int sock) +{ + zassert_equal(listen(sock, MAX_CONNS), + 0, + "listen failed"); +} + +static void test_connect(int sock, struct sockaddr *addr, socklen_t addrlen) +{ + zassert_equal(connect(sock, addr, addrlen), + 0, + "connect failed"); +} + + +static void test_accept(int sock, int *new_sock, struct sockaddr *addr, + socklen_t *addrlen) +{ + zassert_not_null(new_sock, "null newsock"); + + *new_sock = accept(sock, addr, addrlen); + zassert_true(*new_sock >= 0, "accept failed"); +} + +#ifdef CONFIG_POSIX_API +static void test_write(int sock, const void *buf, size_t len) +{ + zassert_equal(write(sock, buf, len), + len, + "write failed"); +} + +static void test_read(int sock) +{ + ssize_t recved = 0; + char rx_buf[30] = {0}; + + recved = read(sock, rx_buf, sizeof(rx_buf)); + zassert_equal(recved, + strlen(TEST_STR_SMALL), + "unexpected received bytes"); + zassert_equal(strncmp(rx_buf, TEST_STR_SMALL, strlen(TEST_STR_SMALL)), + 0, + "unexpected data"); +} +#endif + +static void test_close(int sock) +{ + zassert_equal(close(sock), + 0, + "close failed"); +} + +/* Test that EOF handling works correctly. Should be called with socket + * whose peer socket was closed. + */ +static void test_eof(int sock) +{ + char rx_buf[1]; + ssize_t recved; + + /* Test that EOF properly detected. */ + recved = recv(sock, rx_buf, sizeof(rx_buf), 0); + zassert_equal(recved, 0, ""); + + /* Calling again should be OK. */ + recved = recv(sock, rx_buf, sizeof(rx_buf), 0); + zassert_equal(recved, 0, ""); + + /* Calling when TCP connection is fully torn down should be still OK. */ + k_sleep(TCP_TEARDOWN_TIMEOUT); + recved = recv(sock, rx_buf, sizeof(rx_buf), 0); + zassert_equal(recved, 0, ""); +} + +/* These tests require POSIX API. */ +#ifdef CONFIG_POSIX_API + +void test_v4_write_read(void) +{ + int c_sock; + int s_sock; + int new_sock; + struct sockaddr_in c_saddr; + struct sockaddr_in s_saddr; + struct sockaddr addr; + socklen_t addrlen = sizeof(addr); + + prepare_sock_tcp_v4(CONFIG_NET_CONFIG_MY_IPV4_ADDR, ANY_PORT, + &c_sock, &c_saddr); + prepare_sock_tcp_v4(CONFIG_NET_CONFIG_MY_IPV4_ADDR, SERVER_PORT, + &s_sock, &s_saddr); + + test_bind(s_sock, (struct sockaddr *)&s_saddr, sizeof(s_saddr)); + test_listen(s_sock); + + test_connect(c_sock, (struct sockaddr *)&s_saddr, sizeof(s_saddr)); + test_write(c_sock, TEST_STR_SMALL, strlen(TEST_STR_SMALL)); + + test_accept(s_sock, &new_sock, &addr, &addrlen); + zassert_equal(addrlen, sizeof(struct sockaddr_in), "wrong addrlen"); + + test_read(new_sock); + + test_close(c_sock); + test_eof(new_sock); + + test_close(new_sock); + test_close(s_sock); + + k_sleep(TCP_TEARDOWN_TIMEOUT); +} + +void test_v6_write_read(void) +{ + int c_sock; + int s_sock; + int new_sock; + struct sockaddr_in6 c_saddr; + struct sockaddr_in6 s_saddr; + struct sockaddr addr; + socklen_t addrlen = sizeof(addr); + + prepare_sock_tcp_v6(CONFIG_NET_CONFIG_MY_IPV6_ADDR, ANY_PORT, + &c_sock, &c_saddr); + prepare_sock_tcp_v6(CONFIG_NET_CONFIG_MY_IPV6_ADDR, SERVER_PORT, + &s_sock, &s_saddr); + + test_bind(s_sock, (struct sockaddr *)&s_saddr, sizeof(s_saddr)); + test_listen(s_sock); + + test_connect(c_sock, (struct sockaddr *)&s_saddr, sizeof(s_saddr)); + test_write(c_sock, TEST_STR_SMALL, strlen(TEST_STR_SMALL)); + + test_accept(s_sock, &new_sock, &addr, &addrlen); + zassert_equal(addrlen, sizeof(struct sockaddr_in6), "wrong addrlen"); + + test_read(new_sock); + + test_close(c_sock); + test_eof(new_sock); + + test_close(new_sock); + test_close(s_sock); + + k_sleep(TCP_TEARDOWN_TIMEOUT); +} + +#else /* CONFIG_POSIX_API */ + +void test_v4_write_read(void) +{ + ztest_test_skip(); +} + +void test_v6_write_read(void) +{ + ztest_test_skip(); +} + +#endif /* CONFIG_POSIX_API */ + +void test_main(void) +{ + ztest_test_suite( + socket_tcp_read_write, + ztest_user_unit_test(test_v4_write_read), + ztest_user_unit_test(test_v6_write_read) + ); + + ztest_run_test_suite(socket_tcp_read_write); +} diff --git a/tests/net/socket/tcp_rw/testcase.yaml b/tests/net/socket/tcp_rw/testcase.yaml new file mode 100644 index 0000000000000..1ba4a046875c6 --- /dev/null +++ b/tests/net/socket/tcp_rw/testcase.yaml @@ -0,0 +1,9 @@ +common: + depends_on: netif + min_ram: 32 + tags: net socket userspace +tests: + net.socket.tcp_rw.posix: + extra_configs: + - CONFIG_POSIX_API=y + arch_exclude: posix