[PATCH 0/2] For review - core RTDM, RTnet changes

[PATCH 0/2] For review - core RTDM, RTnet changes

[Philippe Gerum]

I'm sending these changes early on for review because they might have
ramifications, so more eyeballs are needed to make sure nothing was
overlooked.  These changes are prereqs to addressing the device
teardown issue properly, so there is even more incentive to have them
right.

Thanks,

Philippe Gerum (2):
  drivers/net: drop socket-specific module refcounting
  cobalt: switch hand over status to -ENODEV for non-RTDM fd

 kernel/cobalt/rtdm/fd.c                       |  7 +--
 .../drivers/net/stack/include/rtnet_socket.h  | 15 ++----
 kernel/drivers/net/stack/ipv4/icmp.c          |  4 +-
 kernel/drivers/net/stack/socket.c             | 27 ++++-------
 lib/cobalt/rtdm.c                             | 46 +++++++++----------
 5 files changed, 40 insertions(+), 59 deletions(-)

-- 
2.21.0

[PATCH 1/2] drivers/net: drop socket-specific module refcounting

[Philippe Gerum]

RTDM already refcounts rtdm_fd descriptors to prevent unsafe module
unloading while connections are still active. We can remove the legacy
module refcounting done by the generic socket code, since every socket
is covered by an RTDM file descriptor.
---
 .../drivers/net/stack/include/rtnet_socket.h  | 15 +++--------
 kernel/drivers/net/stack/ipv4/icmp.c          |  4 +--
 kernel/drivers/net/stack/socket.c             | 27 ++++++-------------
 3 files changed, 13 insertions(+), 33 deletions(-)

diff --git a/kernel/drivers/net/stack/include/rtnet_socket.h b/kernel/drivers/net/stack/include/rtnet_socket.h
index 351e62c77..0d8850c7f 100644
--- a/kernel/drivers/net/stack/include/rtnet_socket.h
+++ b/kernel/drivers/net/stack/include/rtnet_socket.h
@@ -76,8 +76,6 @@ struct rtsocket {
 	    int                  ifindex;
 	} packet;
     } prot;
-
-    struct module *owner;
 };
 
 
@@ -97,10 +95,7 @@ int rtnet_put_arg(struct rtdm_fd *fd, void *dst,
 #define rt_socket_dereference(sock) \
     rtdm_fd_unlock(rt_socket_fd(sock))
 
-int __rt_socket_init(struct rtdm_fd *fd, unsigned short protocol,
-		struct module *module);
-#define rt_socket_init(fd, proto) \
-    __rt_socket_init(fd, proto, THIS_MODULE)
+int rt_socket_init(struct rtdm_fd *fd, unsigned short protocol);
 
 void rt_socket_cleanup(struct rtdm_fd *fd);
 int rt_socket_common_ioctl(struct rtdm_fd *fd, int request, void __user *arg);
@@ -110,16 +105,12 @@ int rt_socket_select_bind(struct rtdm_fd *fd,
 			  enum rtdm_selecttype type,
 			  unsigned fd_index);
 
-int __rt_bare_socket_init(struct rtdm_fd *fd, unsigned short protocol,
-			unsigned int priority, unsigned int pool_size,
-			struct module *module);
-#define rt_bare_socket_init(fd, proto, prio, pool_sz) \
-    __rt_bare_socket_init(fd, proto, prio, pool_sz, THIS_MODULE)
+int rt_bare_socket_init(struct rtdm_fd *fd, unsigned short protocol,
+			unsigned int priority, unsigned int pool_size);
 
 static inline void rt_bare_socket_cleanup(struct rtsocket *sock)
 {
     rtskb_pool_release(&sock->skb_pool);
-    module_put(sock->owner);
 }
 
 #endif  /* __RTNET_SOCKET_H_ */
diff --git a/kernel/drivers/net/stack/ipv4/icmp.c b/kernel/drivers/net/stack/ipv4/icmp.c
index 58d97cd7f..a944ef6b7 100644
--- a/kernel/drivers/net/stack/ipv4/icmp.c
+++ b/kernel/drivers/net/stack/ipv4/icmp.c
@@ -526,8 +526,8 @@ void __init rt_icmp_init(void)
 {
     int skbs;
 
-    skbs = __rt_bare_socket_init(icmp_fd, IPPROTO_ICMP, RT_ICMP_PRIO,
-			    ICMP_REPLY_POOL_SIZE, NULL);
+    skbs = rt_bare_socket_init(icmp_fd, IPPROTO_ICMP, RT_ICMP_PRIO,
+			    ICMP_REPLY_POOL_SIZE);
     BUG_ON(skbs < 0);
     if (skbs < ICMP_REPLY_POOL_SIZE)
 	printk("RTnet: allocated only %d icmp rtskbs\n", skbs);
diff --git a/kernel/drivers/net/stack/socket.c b/kernel/drivers/net/stack/socket.c
index ce4e4cb46..747db052f 100644
--- a/kernel/drivers/net/stack/socket.c
+++ b/kernel/drivers/net/stack/socket.c
@@ -50,36 +50,27 @@ MODULE_PARM_DESC(socket_rtskbs, "Default number of realtime socket buffers in so
  *  internal socket functions                                           *
  ************************************************************************/
 
-int __rt_bare_socket_init(struct rtdm_fd *fd, unsigned short protocol,
-			unsigned int priority, unsigned int pool_size,
-			struct module *module)
+int rt_bare_socket_init(struct rtdm_fd *fd, unsigned short protocol,
+			unsigned int priority, unsigned int pool_size)
 {
     struct rtsocket *sock = rtdm_fd_to_private(fd);
     int err;
 
-    err = try_module_get(module);
-    if (!err)
-	return -EAFNOSUPPORT;
-
     err = rtskb_pool_init(&sock->skb_pool, pool_size, NULL, fd);
-    if (err < 0) {
-	module_put(module);
+    if (err < 0)
 	return err;
-    }
 
     sock->protocol = protocol;
     sock->priority = priority;
-    sock->owner = module;
 
     return err;
 }
-EXPORT_SYMBOL_GPL(__rt_bare_socket_init);
+EXPORT_SYMBOL_GPL(rt_bare_socket_init);
 
 /***
  *  rt_socket_init - initialises a new socket structure
  */
-int __rt_socket_init(struct rtdm_fd *fd, unsigned short protocol,
-		struct module *module)
+int rt_socket_init(struct rtdm_fd *fd, unsigned short protocol)
 {
     struct rtsocket *sock = rtdm_fd_to_private(fd);
     unsigned int    pool_size;
@@ -94,10 +85,10 @@ int __rt_socket_init(struct rtdm_fd *fd, unsigned short protocol,
     rtdm_lock_init(&sock->param_lock);
     rtdm_sem_init(&sock->pending_sem, 0);
 
-    pool_size = __rt_bare_socket_init(fd, protocol,
+    pool_size = rt_bare_socket_init(fd, protocol,
 				    RTSKB_PRIO_VALUE(SOCK_DEF_PRIO,
 						    RTSKB_DEF_RT_CHANNEL),
-				    socket_rtskbs, module);
+				    socket_rtskbs);
     sock->pool_size = pool_size;
     mutex_init(&sock->pool_nrt_lock);
 
@@ -112,7 +103,7 @@ int __rt_socket_init(struct rtdm_fd *fd, unsigned short protocol,
 
     return 0;
 }
-EXPORT_SYMBOL_GPL(__rt_socket_init);
+EXPORT_SYMBOL_GPL(rt_socket_init);
 
 
 /***
@@ -133,8 +124,6 @@ void rt_socket_cleanup(struct rtdm_fd *fd)
 	rtskb_pool_release(&sock->skb_pool);
 
     mutex_unlock(&sock->pool_nrt_lock);
-
-    module_put(sock->owner);
 }
 EXPORT_SYMBOL_GPL(rt_socket_cleanup);
 
-- 
2.21.0

[PATCH 2/2] cobalt: switch hand over status to -ENODEV for non-RTDM fd

[Philippe Gerum]

Having the RTDM core return -EBADF to indicate that it does not manage
a file descriptor is a problem, as several drivers also raise this
error to notify userland about an aborted wait due to a connection
being dismantled (e.g. RTnet). In this case, libcobalt ends up
forwarding the aborted request to the glibc, which is wrong.

Switch from -EBADF to -ENODEV to notify userland that RTDM does not
manage a file descriptor, which cannot conflict with any sensible
return from active I/O operations. This is also consistent with the
status RTDM currently returns to notify that it cannot handle a device
open request.
---
 kernel/cobalt/rtdm/fd.c |  7 ++++---
 lib/cobalt/rtdm.c       | 46 ++++++++++++++++++++---------------------
 2 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/kernel/cobalt/rtdm/fd.c b/kernel/cobalt/rtdm/fd.c
index f3b6444c3..75c4acf28 100644
--- a/kernel/cobalt/rtdm/fd.c
+++ b/kernel/cobalt/rtdm/fd.c
@@ -204,8 +204,9 @@ int rtdm_fd_register(struct rtdm_fd *fd, int ufd)
  * @param[in] ufd User-side file descriptor
  * @param[in] magic Magic word for lookup validation
  *
- * @return Pointer to the RTDM file descriptor matching @a ufd, or
- * ERR_PTR(-EBADF).
+ * @return Pointer to the RTDM file descriptor matching @a
+ * ufd. Otherwise ERR_PTR(-ENODEV) is returned if the use-space handle
+ * is either invalid or not managed by RTDM.
  *
  * @note The file descriptor returned must be later released by a call
  * to rtdm_fd_put().
@@ -221,7 +222,7 @@ struct rtdm_fd *rtdm_fd_get(int ufd, unsigned int magic)
 	xnlock_get_irqsave(&fdtree_lock, s);
 	fd = fetch_fd(p, ufd);
 	if (fd == NULL || (magic != 0 && fd->magic != magic)) {
-		fd = ERR_PTR(-EBADF);
+		fd = ERR_PTR(-ENODEV);
 		goto out;
 	}
 
diff --git a/lib/cobalt/rtdm.c b/lib/cobalt/rtdm.c
index 176210ddc..506302d26 100644
--- a/lib/cobalt/rtdm.c
+++ b/lib/cobalt/rtdm.c
@@ -123,7 +123,7 @@ COBALT_IMPL(int, close, (int fd))
 
 	pthread_setcanceltype(oldtype, NULL);
 
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(close(fd));
@@ -154,7 +154,7 @@ COBALT_IMPL(int, fcntl, (int fd, int cmd, ...))
 
 	ret = XENOMAI_SYSCALL3(sc_cobalt_fcntl, fd, cmd, arg);
 
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(fcntl(fd, cmd, arg));
@@ -171,7 +171,7 @@ COBALT_IMPL(int, ioctl, (int fd, unsigned int request, ...))
 	va_end(ap);
 
 	ret = do_ioctl(fd, request, arg);
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(ioctl(fd, request, arg));
@@ -187,7 +187,7 @@ COBALT_IMPL(ssize_t, read, (int fd, void *buf, size_t nbyte))
 
 	pthread_setcanceltype(oldtype, NULL);
 
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(read(fd, buf, nbyte));
@@ -203,7 +203,7 @@ COBALT_IMPL(ssize_t, write, (int fd, const void *buf, size_t nbyte))
 
 	pthread_setcanceltype(oldtype, NULL);
 
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(write(fd, buf, nbyte));
@@ -227,7 +227,7 @@ COBALT_IMPL(ssize_t, recvmsg, (int fd, struct msghdr *msg, int flags))
 	int ret;
 
 	ret = do_recvmsg(fd, msg, flags);
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(recvmsg(fd, msg, flags));
@@ -244,7 +244,7 @@ COBALT_IMPL(int, recvmmsg, (int fd, struct mmsghdr *msgvec, unsigned int vlen,
 
 	pthread_setcanceltype(oldtype, NULL);
 
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(recvmmsg(fd, msgvec, vlen, flags, timeout));
@@ -268,7 +268,7 @@ COBALT_IMPL(ssize_t, sendmsg, (int fd, const struct msghdr *msg, int flags))
 	int ret;
 
 	ret = do_sendmsg(fd, msg, flags);
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(sendmsg(fd, msg, flags));
@@ -285,7 +285,7 @@ COBALT_IMPL(int, sendmmsg, (int fd, struct mmsghdr *msgvec,
 
 	pthread_setcanceltype(oldtype, NULL);
 
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(sendmmsg(fd, msgvec, vlen, flags));
@@ -309,7 +309,7 @@ COBALT_IMPL(ssize_t, recvfrom, (int fd, void *buf, size_t len, int flags,
 	int ret;
 
 	ret = do_recvmsg(fd, &msg, flags);
-	if (ret != -EBADF && ret != -ENOSYS) {
+	if (ret != -ENODEV && ret != -ENOSYS) {
 		if (ret < 0)
 			return set_errno(ret);
 
@@ -340,7 +340,7 @@ COBALT_IMPL(ssize_t, sendto, (int fd, const void *buf, size_t len, int flags,
 	int ret;
 
 	ret = do_sendmsg(fd, &msg, flags);
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(sendto(fd, buf, len, flags, to, tolen));
@@ -363,7 +363,7 @@ COBALT_IMPL(ssize_t, recv, (int fd, void *buf, size_t len, int flags))
 	int ret;
 
 	ret = do_recvmsg(fd, &msg, flags);
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(recv(fd, buf, len, flags));
@@ -386,7 +386,7 @@ COBALT_IMPL(ssize_t, send, (int fd, const void *buf, size_t len, int flags))
 	int ret;
 
 	ret = do_sendmsg(fd, &msg, flags);
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(send(fd, buf, len, flags));
@@ -399,7 +399,7 @@ COBALT_IMPL(int, getsockopt, (int fd, int level, int optname, void *optval,
 	int ret;
 
 	ret = do_ioctl(fd, _RTIOC_GETSOCKOPT, &args);
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(getsockopt(fd, level, optname, optval, optlen));
@@ -414,7 +414,7 @@ COBALT_IMPL(int, setsockopt, (int fd, int level, int optname, const void *optval
 	int ret;
 
 	ret = do_ioctl(fd, _RTIOC_SETSOCKOPT, &args);
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(setsockopt(fd, level, optname, optval, optlen));
@@ -426,7 +426,7 @@ COBALT_IMPL(int, bind, (int fd, const struct sockaddr *my_addr, socklen_t addrle
 	int ret;
 
 	ret = do_ioctl(fd, _RTIOC_BIND, &args);
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(bind(fd, my_addr, addrlen));
@@ -438,7 +438,7 @@ COBALT_IMPL(int, connect, (int fd, const struct sockaddr *serv_addr, socklen_t a
 	int ret;
 
 	ret = do_ioctl(fd, _RTIOC_CONNECT, &args);
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(connect(fd, serv_addr, addrlen));
@@ -449,7 +449,7 @@ COBALT_IMPL(int, listen, (int fd, int backlog))
 	int ret;
 
 	ret = do_ioctl(fd, _RTIOC_LISTEN, (void *)(long)backlog);
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(listen(fd, backlog));
@@ -461,7 +461,7 @@ COBALT_IMPL(int, accept, (int fd, struct sockaddr *addr, socklen_t *addrlen))
 	int ret;
 
 	ret = do_ioctl(fd, _RTIOC_ACCEPT, &args);
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(accept(fd, addr, addrlen));
@@ -473,7 +473,7 @@ COBALT_IMPL(int, getsockname, (int fd, struct sockaddr *name, socklen_t *namelen
 	int ret;
 
 	ret = do_ioctl(fd, _RTIOC_GETSOCKNAME, &args);
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(getsockname(fd, name, namelen));
@@ -485,7 +485,7 @@ COBALT_IMPL(int, getpeername, (int fd, struct sockaddr *name, socklen_t *namelen
 	int ret;
 
 	ret = do_ioctl(fd, _RTIOC_GETPEERNAME, &args);
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(getpeername(fd, name, namelen));
@@ -496,7 +496,7 @@ COBALT_IMPL(int, shutdown, (int fd, int how))
 	int ret;
 
 	ret = do_ioctl(fd, _RTIOC_SHUTDOWN, (void *)(long)how);
-	if (ret != -EBADF && ret != -ENOSYS)
+	if (ret != -ENODEV && ret != -ENOSYS)
 		return set_errno(ret);
 
 	return __STD(shutdown(fd, how));
@@ -518,7 +518,7 @@ COBALT_IMPL(void *, mmap64, (void *addr, size_t length, int prot, int flags,
 	rma.flags = flags;
 
 	ret = XENOMAI_SYSCALL3(sc_cobalt_mmap, fd, &rma, &addr);
-	if (ret != -EBADF && ret != -ENOSYS) {
+	if (ret != -ENODEV && ret != -ENOSYS) {
 		ret = set_errno(ret);
 		if (ret)
 			return MAP_FAILED;
-- 
2.21.0

Re: [PATCH 1/2] drivers/net: drop socket-specific module refcounting

[Jan Kiszka]

On 20.06.19 19:29, Philippe Gerum via Xenomai wrote:
> RTDM already refcounts rtdm_fd descriptors to prevent unsafe module
> unloading while connections are still active. We can remove the legacy
> module refcounting done by the generic socket code, since every socket
> is covered by an RTDM file descriptor.
> ---
>   .../drivers/net/stack/include/rtnet_socket.h  | 15 +++--------
>   kernel/drivers/net/stack/ipv4/icmp.c          |  4 +--
>   kernel/drivers/net/stack/socket.c             | 27 ++++++-------------
>   3 files changed, 13 insertions(+), 33 deletions(-)
> 
> diff --git a/kernel/drivers/net/stack/include/rtnet_socket.h b/kernel/drivers/net/stack/include/rtnet_socket.h
> index 351e62c77..0d8850c7f 100644
> --- a/kernel/drivers/net/stack/include/rtnet_socket.h
> +++ b/kernel/drivers/net/stack/include/rtnet_socket.h
> @@ -76,8 +76,6 @@ struct rtsocket {
>   	    int                  ifindex;
>   	} packet;
>       } prot;
> -
> -    struct module *owner;
>   };
>   
>   
> @@ -97,10 +95,7 @@ int rtnet_put_arg(struct rtdm_fd *fd, void *dst,
>   #define rt_socket_dereference(sock) \
>       rtdm_fd_unlock(rt_socket_fd(sock))
>   
> -int __rt_socket_init(struct rtdm_fd *fd, unsigned short protocol,
> -		struct module *module);
> -#define rt_socket_init(fd, proto) \
> -    __rt_socket_init(fd, proto, THIS_MODULE)
> +int rt_socket_init(struct rtdm_fd *fd, unsigned short protocol);
>   
>   void rt_socket_cleanup(struct rtdm_fd *fd);
>   int rt_socket_common_ioctl(struct rtdm_fd *fd, int request, void __user *arg);
> @@ -110,16 +105,12 @@ int rt_socket_select_bind(struct rtdm_fd *fd,
>   			  enum rtdm_selecttype type,
>   			  unsigned fd_index);
>   
> -int __rt_bare_socket_init(struct rtdm_fd *fd, unsigned short protocol,
> -			unsigned int priority, unsigned int pool_size,
> -			struct module *module);
> -#define rt_bare_socket_init(fd, proto, prio, pool_sz) \
> -    __rt_bare_socket_init(fd, proto, prio, pool_sz, THIS_MODULE)
> +int rt_bare_socket_init(struct rtdm_fd *fd, unsigned short protocol,
> +			unsigned int priority, unsigned int pool_size);
>   
>   static inline void rt_bare_socket_cleanup(struct rtsocket *sock)
>   {
>       rtskb_pool_release(&sock->skb_pool);
> -    module_put(sock->owner);
>   }
>   
>   #endif  /* __RTNET_SOCKET_H_ */
> diff --git a/kernel/drivers/net/stack/ipv4/icmp.c b/kernel/drivers/net/stack/ipv4/icmp.c
> index 58d97cd7f..a944ef6b7 100644
> --- a/kernel/drivers/net/stack/ipv4/icmp.c
> +++ b/kernel/drivers/net/stack/ipv4/icmp.c
> @@ -526,8 +526,8 @@ void __init rt_icmp_init(void)
>   {
>       int skbs;
>   
> -    skbs = __rt_bare_socket_init(icmp_fd, IPPROTO_ICMP, RT_ICMP_PRIO,
> -			    ICMP_REPLY_POOL_SIZE, NULL);
> +    skbs = rt_bare_socket_init(icmp_fd, IPPROTO_ICMP, RT_ICMP_PRIO,
> +			    ICMP_REPLY_POOL_SIZE);
>       BUG_ON(skbs < 0);
>       if (skbs < ICMP_REPLY_POOL_SIZE)
>   	printk("RTnet: allocated only %d icmp rtskbs\n", skbs);
> diff --git a/kernel/drivers/net/stack/socket.c b/kernel/drivers/net/stack/socket.c
> index ce4e4cb46..747db052f 100644
> --- a/kernel/drivers/net/stack/socket.c
> +++ b/kernel/drivers/net/stack/socket.c
> @@ -50,36 +50,27 @@ MODULE_PARM_DESC(socket_rtskbs, "Default number of realtime socket buffers in so
>    *  internal socket functions                                           *
>    ************************************************************************/
>   
> -int __rt_bare_socket_init(struct rtdm_fd *fd, unsigned short protocol,
> -			unsigned int priority, unsigned int pool_size,
> -			struct module *module)
> +int rt_bare_socket_init(struct rtdm_fd *fd, unsigned short protocol,
> +			unsigned int priority, unsigned int pool_size)
>   {
>       struct rtsocket *sock = rtdm_fd_to_private(fd);
>       int err;
>   
> -    err = try_module_get(module);
> -    if (!err)
> -	return -EAFNOSUPPORT;
> -
>       err = rtskb_pool_init(&sock->skb_pool, pool_size, NULL, fd);
> -    if (err < 0) {
> -	module_put(module);
> +    if (err < 0)
>   	return err;
> -    }
>   
>       sock->protocol = protocol;
>       sock->priority = priority;
> -    sock->owner = module;
>   
>       return err;
>   }
> -EXPORT_SYMBOL_GPL(__rt_bare_socket_init);
> +EXPORT_SYMBOL_GPL(rt_bare_socket_init);
>   
>   /***
>    *  rt_socket_init - initialises a new socket structure
>    */
> -int __rt_socket_init(struct rtdm_fd *fd, unsigned short protocol,
> -		struct module *module)
> +int rt_socket_init(struct rtdm_fd *fd, unsigned short protocol)
>   {
>       struct rtsocket *sock = rtdm_fd_to_private(fd);
>       unsigned int    pool_size;
> @@ -94,10 +85,10 @@ int __rt_socket_init(struct rtdm_fd *fd, unsigned short protocol,
>       rtdm_lock_init(&sock->param_lock);
>       rtdm_sem_init(&sock->pending_sem, 0);
>   
> -    pool_size = __rt_bare_socket_init(fd, protocol,
> +    pool_size = rt_bare_socket_init(fd, protocol,
>   				    RTSKB_PRIO_VALUE(SOCK_DEF_PRIO,
>   						    RTSKB_DEF_RT_CHANNEL),
> -				    socket_rtskbs, module);
> +				    socket_rtskbs);
>       sock->pool_size = pool_size;
>       mutex_init(&sock->pool_nrt_lock);
>   
> @@ -112,7 +103,7 @@ int __rt_socket_init(struct rtdm_fd *fd, unsigned short protocol,
>   
>       return 0;
>   }
> -EXPORT_SYMBOL_GPL(__rt_socket_init);
> +EXPORT_SYMBOL_GPL(rt_socket_init);
>   
>   
>   /***
> @@ -133,8 +124,6 @@ void rt_socket_cleanup(struct rtdm_fd *fd)
>   	rtskb_pool_release(&sock->skb_pool);
>   
>       mutex_unlock(&sock->pool_nrt_lock);
> -
> -    module_put(sock->owner);
>   }
>   EXPORT_SYMBOL_GPL(rt_socket_cleanup);
>   
> 

Looks reasonable. Maybe you can additionally clarify in the commit log that 
rtdm_dev_unregister will now prevent unloading under open sockets.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] cobalt: switch hand over status to -ENODEV for non-RTDM fd

[Jan Kiszka]

On 20.06.19 19:29, Philippe Gerum via Xenomai wrote:
> Having the RTDM core return -EBADF to indicate that it does not manage
> a file descriptor is a problem, as several drivers also raise this
> error to notify userland about an aborted wait due to a connection
> being dismantled (e.g. RTnet). In this case, libcobalt ends up
> forwarding the aborted request to the glibc, which is wrong.
> 
> Switch from -EBADF to -ENODEV to notify userland that RTDM does not
> manage a file descriptor, which cannot conflict with any sensible
> return from active I/O operations. This is also consistent with the
> status RTDM currently returns to notify that it cannot handle a device
> open request.
> ---
>   kernel/cobalt/rtdm/fd.c |  7 ++++---
>   lib/cobalt/rtdm.c       | 46 ++++++++++++++++++++---------------------
>   2 files changed, 27 insertions(+), 26 deletions(-)
> 
> diff --git a/kernel/cobalt/rtdm/fd.c b/kernel/cobalt/rtdm/fd.c
> index f3b6444c3..75c4acf28 100644
> --- a/kernel/cobalt/rtdm/fd.c
> +++ b/kernel/cobalt/rtdm/fd.c
> @@ -204,8 +204,9 @@ int rtdm_fd_register(struct rtdm_fd *fd, int ufd)
>    * @param[in] ufd User-side file descriptor
>    * @param[in] magic Magic word for lookup validation
>    *
> - * @return Pointer to the RTDM file descriptor matching @a ufd, or
> - * ERR_PTR(-EBADF).
> + * @return Pointer to the RTDM file descriptor matching @a
> + * ufd. Otherwise ERR_PTR(-ENODEV) is returned if the use-space handle
> + * is either invalid or not managed by RTDM.
>    *
>    * @note The file descriptor returned must be later released by a call
>    * to rtdm_fd_put().
> @@ -221,7 +222,7 @@ struct rtdm_fd *rtdm_fd_get(int ufd, unsigned int magic)
>   	xnlock_get_irqsave(&fdtree_lock, s);
>   	fd = fetch_fd(p, ufd);
>   	if (fd == NULL || (magic != 0 && fd->magic != magic)) {
> -		fd = ERR_PTR(-EBADF);
> +		fd = ERR_PTR(-ENODEV);
>   		goto out;
>   	}
>   
> diff --git a/lib/cobalt/rtdm.c b/lib/cobalt/rtdm.c
> index 176210ddc..506302d26 100644
> --- a/lib/cobalt/rtdm.c
> +++ b/lib/cobalt/rtdm.c
> @@ -123,7 +123,7 @@ COBALT_IMPL(int, close, (int fd))
>   
>   	pthread_setcanceltype(oldtype, NULL);
>   
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(close(fd));
> @@ -154,7 +154,7 @@ COBALT_IMPL(int, fcntl, (int fd, int cmd, ...))
>   
>   	ret = XENOMAI_SYSCALL3(sc_cobalt_fcntl, fd, cmd, arg);
>   
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(fcntl(fd, cmd, arg));
> @@ -171,7 +171,7 @@ COBALT_IMPL(int, ioctl, (int fd, unsigned int request, ...))
>   	va_end(ap);
>   
>   	ret = do_ioctl(fd, request, arg);
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(ioctl(fd, request, arg));
> @@ -187,7 +187,7 @@ COBALT_IMPL(ssize_t, read, (int fd, void *buf, size_t nbyte))
>   
>   	pthread_setcanceltype(oldtype, NULL);
>   
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(read(fd, buf, nbyte));
> @@ -203,7 +203,7 @@ COBALT_IMPL(ssize_t, write, (int fd, const void *buf, size_t nbyte))
>   
>   	pthread_setcanceltype(oldtype, NULL);
>   
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(write(fd, buf, nbyte));
> @@ -227,7 +227,7 @@ COBALT_IMPL(ssize_t, recvmsg, (int fd, struct msghdr *msg, int flags))
>   	int ret;
>   
>   	ret = do_recvmsg(fd, msg, flags);
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(recvmsg(fd, msg, flags));
> @@ -244,7 +244,7 @@ COBALT_IMPL(int, recvmmsg, (int fd, struct mmsghdr *msgvec, unsigned int vlen,
>   
>   	pthread_setcanceltype(oldtype, NULL);
>   
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(recvmmsg(fd, msgvec, vlen, flags, timeout));
> @@ -268,7 +268,7 @@ COBALT_IMPL(ssize_t, sendmsg, (int fd, const struct msghdr *msg, int flags))
>   	int ret;
>   
>   	ret = do_sendmsg(fd, msg, flags);
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(sendmsg(fd, msg, flags));
> @@ -285,7 +285,7 @@ COBALT_IMPL(int, sendmmsg, (int fd, struct mmsghdr *msgvec,
>   
>   	pthread_setcanceltype(oldtype, NULL);
>   
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(sendmmsg(fd, msgvec, vlen, flags));
> @@ -309,7 +309,7 @@ COBALT_IMPL(ssize_t, recvfrom, (int fd, void *buf, size_t len, int flags,
>   	int ret;
>   
>   	ret = do_recvmsg(fd, &msg, flags);
> -	if (ret != -EBADF && ret != -ENOSYS) {
> +	if (ret != -ENODEV && ret != -ENOSYS) {
>   		if (ret < 0)
>   			return set_errno(ret);
>   
> @@ -340,7 +340,7 @@ COBALT_IMPL(ssize_t, sendto, (int fd, const void *buf, size_t len, int flags,
>   	int ret;
>   
>   	ret = do_sendmsg(fd, &msg, flags);
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(sendto(fd, buf, len, flags, to, tolen));
> @@ -363,7 +363,7 @@ COBALT_IMPL(ssize_t, recv, (int fd, void *buf, size_t len, int flags))
>   	int ret;
>   
>   	ret = do_recvmsg(fd, &msg, flags);
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(recv(fd, buf, len, flags));
> @@ -386,7 +386,7 @@ COBALT_IMPL(ssize_t, send, (int fd, const void *buf, size_t len, int flags))
>   	int ret;
>   
>   	ret = do_sendmsg(fd, &msg, flags);
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(send(fd, buf, len, flags));
> @@ -399,7 +399,7 @@ COBALT_IMPL(int, getsockopt, (int fd, int level, int optname, void *optval,
>   	int ret;
>   
>   	ret = do_ioctl(fd, _RTIOC_GETSOCKOPT, &args);
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(getsockopt(fd, level, optname, optval, optlen));
> @@ -414,7 +414,7 @@ COBALT_IMPL(int, setsockopt, (int fd, int level, int optname, const void *optval
>   	int ret;
>   
>   	ret = do_ioctl(fd, _RTIOC_SETSOCKOPT, &args);
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(setsockopt(fd, level, optname, optval, optlen));
> @@ -426,7 +426,7 @@ COBALT_IMPL(int, bind, (int fd, const struct sockaddr *my_addr, socklen_t addrle
>   	int ret;
>   
>   	ret = do_ioctl(fd, _RTIOC_BIND, &args);
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(bind(fd, my_addr, addrlen));
> @@ -438,7 +438,7 @@ COBALT_IMPL(int, connect, (int fd, const struct sockaddr *serv_addr, socklen_t a
>   	int ret;
>   
>   	ret = do_ioctl(fd, _RTIOC_CONNECT, &args);
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(connect(fd, serv_addr, addrlen));
> @@ -449,7 +449,7 @@ COBALT_IMPL(int, listen, (int fd, int backlog))
>   	int ret;
>   
>   	ret = do_ioctl(fd, _RTIOC_LISTEN, (void *)(long)backlog);
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(listen(fd, backlog));
> @@ -461,7 +461,7 @@ COBALT_IMPL(int, accept, (int fd, struct sockaddr *addr, socklen_t *addrlen))
>   	int ret;
>   
>   	ret = do_ioctl(fd, _RTIOC_ACCEPT, &args);
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(accept(fd, addr, addrlen));
> @@ -473,7 +473,7 @@ COBALT_IMPL(int, getsockname, (int fd, struct sockaddr *name, socklen_t *namelen
>   	int ret;
>   
>   	ret = do_ioctl(fd, _RTIOC_GETSOCKNAME, &args);
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(getsockname(fd, name, namelen));
> @@ -485,7 +485,7 @@ COBALT_IMPL(int, getpeername, (int fd, struct sockaddr *name, socklen_t *namelen
>   	int ret;
>   
>   	ret = do_ioctl(fd, _RTIOC_GETPEERNAME, &args);
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(getpeername(fd, name, namelen));
> @@ -496,7 +496,7 @@ COBALT_IMPL(int, shutdown, (int fd, int how))
>   	int ret;
>   
>   	ret = do_ioctl(fd, _RTIOC_SHUTDOWN, (void *)(long)how);
> -	if (ret != -EBADF && ret != -ENOSYS)
> +	if (ret != -ENODEV && ret != -ENOSYS)
>   		return set_errno(ret);
>   
>   	return __STD(shutdown(fd, how));
> @@ -518,7 +518,7 @@ COBALT_IMPL(void *, mmap64, (void *addr, size_t length, int prot, int flags,
>   	rma.flags = flags;
>   
>   	ret = XENOMAI_SYSCALL3(sc_cobalt_mmap, fd, &rma, &addr);
> -	if (ret != -EBADF && ret != -ENOSYS) {
> +	if (ret != -ENODEV && ret != -ENOSYS) {
>   		ret = set_errno(ret);
>   		if (ret)
>   			return MAP_FAILED;
> 

Looks reasonable, just wondering if we didn't run into other cases where ENODEV 
is returned by the driver already. I would suggest using some uncommon error 
code instead.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

RE: [PATCH 2/2] cobalt: switch hand over status to -ENODEV for non-RTDM fd

[Lange Norbert]

I ran into a rather big issue with linux filehandles
I use Xenomai master on ipipe-core-4.19.60-x86-5 with those patches,
(can't be 100% sure its not some kernel/userspace conflict, but I doubt it)

What happens is that upon a __cobalt_close with a linux filehande, the
syscall sc_cobalt_close returns EBADF, but that means the libc close will
never be tried and filehandles are leaking like mad.

----------------------------test.c
int fileread(const char *pFilename, int *pErrno)
{
    int fd = open(pFilename, O_RDONLY | FILEIO_OPENOPTS);
    bool isOk = true;

    if (fd < 0) {
        *pErrno = errno;
        return -1;
    }

    char buffer[1024];

    ssize_t result = read(fd, buffer, sizeof(buffer));

    if (result < 0) {
        *pErrno = errno;
        isOk = false;
    }

    close(fd);
    return isOk ? (int)(result) : -1;
}


int main(int argc, char **argv)
{
    if (argc != 2)
        return -1;

    int err;

    for (;;) {
        if(fileread(argv[1], &err) < 0)
        {
            perror("read failed: ");
            break;
        }
        usleep(100);
    }
}
---------------------------------------

Kind regards, Norbert

> -----Original Message-----
> From: Xenomai <xenomai-bounces@xenomai.org> On Behalf Of Philippe
> Gerum via Xenomai
> Sent: Donnerstag, 20. Juni 2019 19:30
> To: xenomai@xenomai.org
> Subject: [PATCH 2/2] cobalt: switch hand over status to -ENODEV for non-
> RTDM fd
>
> E-MAIL FROM A NON-ANDRITZ SOURCE: AS A SECURITY MEASURE, PLEASE
> EXERCISE CAUTION WITH E-MAIL CONTENT AND ANY LINKS OR
> ATTACHMENTS.
>
>
> Having the RTDM core return -EBADF to indicate that it does not manage a
> file descriptor is a problem, as several drivers also raise this error to notify
> userland about an aborted wait due to a connection being dismantled (e.g.
> RTnet). In this case, libcobalt ends up forwarding the aborted request to the
> glibc, which is wrong.
>
> Switch from -EBADF to -ENODEV to notify userland that RTDM does not
> manage a file descriptor, which cannot conflict with any sensible return from
> active I/O operations. This is also consistent with the status RTDM currently
> returns to notify that it cannot handle a device open request.
> ---
>  kernel/cobalt/rtdm/fd.c |  7 ++++---
>  lib/cobalt/rtdm.c       | 46 ++++++++++++++++++++---------------------
>  2 files changed, 27 insertions(+), 26 deletions(-)
>
> diff --git a/kernel/cobalt/rtdm/fd.c b/kernel/cobalt/rtdm/fd.c index
> f3b6444c3..75c4acf28 100644
> --- a/kernel/cobalt/rtdm/fd.c
> +++ b/kernel/cobalt/rtdm/fd.c
> @@ -204,8 +204,9 @@ int rtdm_fd_register(struct rtdm_fd *fd, int ufd)
>   * @param[in] ufd User-side file descriptor
>   * @param[in] magic Magic word for lookup validation
>   *
> - * @return Pointer to the RTDM file descriptor matching @a ufd, or
> - * ERR_PTR(-EBADF).
> + * @return Pointer to the RTDM file descriptor matching @a
> + * ufd. Otherwise ERR_PTR(-ENODEV) is returned if the use-space handle
> + * is either invalid or not managed by RTDM.
>   *
>   * @note The file descriptor returned must be later released by a call
>   * to rtdm_fd_put().
> @@ -221,7 +222,7 @@ struct rtdm_fd *rtdm_fd_get(int ufd, unsigned int
> magic)
>         xnlock_get_irqsave(&fdtree_lock, s);
>         fd = fetch_fd(p, ufd);
>         if (fd == NULL || (magic != 0 && fd->magic != magic)) {
> -               fd = ERR_PTR(-EBADF);
> +               fd = ERR_PTR(-ENODEV);
>                 goto out;
>         }
>
> diff --git a/lib/cobalt/rtdm.c b/lib/cobalt/rtdm.c index 176210ddc..506302d26
> 100644
> --- a/lib/cobalt/rtdm.c
> +++ b/lib/cobalt/rtdm.c
> @@ -123,7 +123,7 @@ COBALT_IMPL(int, close, (int fd))
>
>         pthread_setcanceltype(oldtype, NULL);
>
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(close(fd));
> @@ -154,7 +154,7 @@ COBALT_IMPL(int, fcntl, (int fd, int cmd, ...))
>
>         ret = XENOMAI_SYSCALL3(sc_cobalt_fcntl, fd, cmd, arg);
>
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(fcntl(fd, cmd, arg)); @@ -171,7 +171,7 @@
> COBALT_IMPL(int, ioctl, (int fd, unsigned int request, ...))
>         va_end(ap);
>
>         ret = do_ioctl(fd, request, arg);
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(ioctl(fd, request, arg)); @@ -187,7 +187,7 @@
> COBALT_IMPL(ssize_t, read, (int fd, void *buf, size_t nbyte))
>
>         pthread_setcanceltype(oldtype, NULL);
>
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(read(fd, buf, nbyte)); @@ -203,7 +203,7 @@
> COBALT_IMPL(ssize_t, write, (int fd, const void *buf, size_t nbyte))
>
>         pthread_setcanceltype(oldtype, NULL);
>
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(write(fd, buf, nbyte)); @@ -227,7 +227,7 @@
> COBALT_IMPL(ssize_t, recvmsg, (int fd, struct msghdr *msg, int flags))
>         int ret;
>
>         ret = do_recvmsg(fd, msg, flags);
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(recvmsg(fd, msg, flags)); @@ -244,7 +244,7 @@
> COBALT_IMPL(int, recvmmsg, (int fd, struct mmsghdr *msgvec, unsigned int
> vlen,
>
>         pthread_setcanceltype(oldtype, NULL);
>
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(recvmmsg(fd, msgvec, vlen, flags, timeout)); @@ -268,7
> +268,7 @@ COBALT_IMPL(ssize_t, sendmsg, (int fd, const struct msghdr
> *msg, int flags))
>         int ret;
>
>         ret = do_sendmsg(fd, msg, flags);
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(sendmsg(fd, msg, flags)); @@ -285,7 +285,7 @@
> COBALT_IMPL(int, sendmmsg, (int fd, struct mmsghdr *msgvec,
>
>         pthread_setcanceltype(oldtype, NULL);
>
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(sendmmsg(fd, msgvec, vlen, flags)); @@ -309,7 +309,7
> @@ COBALT_IMPL(ssize_t, recvfrom, (int fd, void *buf, size_t len, int flags,
>         int ret;
>
>         ret = do_recvmsg(fd, &msg, flags);
> -       if (ret != -EBADF && ret != -ENOSYS) {
> +       if (ret != -ENODEV && ret != -ENOSYS) {
>                 if (ret < 0)
>                         return set_errno(ret);
>
> @@ -340,7 +340,7 @@ COBALT_IMPL(ssize_t, sendto, (int fd, const void
> *buf, size_t len, int flags,
>         int ret;
>
>         ret = do_sendmsg(fd, &msg, flags);
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(sendto(fd, buf, len, flags, to, tolen)); @@ -363,7 +363,7
> @@ COBALT_IMPL(ssize_t, recv, (int fd, void *buf, size_t len, int flags))
>         int ret;
>
>         ret = do_recvmsg(fd, &msg, flags);
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(recv(fd, buf, len, flags)); @@ -386,7 +386,7 @@
> COBALT_IMPL(ssize_t, send, (int fd, const void *buf, size_t len, int flags))
>         int ret;
>
>         ret = do_sendmsg(fd, &msg, flags);
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(send(fd, buf, len, flags)); @@ -399,7 +399,7 @@
> COBALT_IMPL(int, getsockopt, (int fd, int level, int optname, void *optval,
>         int ret;
>
>         ret = do_ioctl(fd, _RTIOC_GETSOCKOPT, &args);
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(getsockopt(fd, level, optname, optval, optlen)); @@ -414,7
> +414,7 @@ COBALT_IMPL(int, setsockopt, (int fd, int level, int optname,
> const void *optval
>         int ret;
>
>         ret = do_ioctl(fd, _RTIOC_SETSOCKOPT, &args);
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(setsockopt(fd, level, optname, optval, optlen)); @@ -426,7
> +426,7 @@ COBALT_IMPL(int, bind, (int fd, const struct sockaddr *my_addr,
> socklen_t addrle
>         int ret;
>
>         ret = do_ioctl(fd, _RTIOC_BIND, &args);
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(bind(fd, my_addr, addrlen)); @@ -438,7 +438,7 @@
> COBALT_IMPL(int, connect, (int fd, const struct sockaddr *serv_addr,
> socklen_t a
>         int ret;
>
>         ret = do_ioctl(fd, _RTIOC_CONNECT, &args);
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(connect(fd, serv_addr, addrlen)); @@ -449,7 +449,7 @@
> COBALT_IMPL(int, listen, (int fd, int backlog))
>         int ret;
>
>         ret = do_ioctl(fd, _RTIOC_LISTEN, (void *)(long)backlog);
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(listen(fd, backlog)); @@ -461,7 +461,7 @@
> COBALT_IMPL(int, accept, (int fd, struct sockaddr *addr, socklen_t
> *addrlen))
>         int ret;
>
>         ret = do_ioctl(fd, _RTIOC_ACCEPT, &args);
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(accept(fd, addr, addrlen)); @@ -473,7 +473,7 @@
> COBALT_IMPL(int, getsockname, (int fd, struct sockaddr *name, socklen_t
> *namelen
>         int ret;
>
>         ret = do_ioctl(fd, _RTIOC_GETSOCKNAME, &args);
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(getsockname(fd, name, namelen)); @@ -485,7 +485,7 @@
> COBALT_IMPL(int, getpeername, (int fd, struct sockaddr *name, socklen_t
> *namelen
>         int ret;
>
>         ret = do_ioctl(fd, _RTIOC_GETPEERNAME, &args);
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(getpeername(fd, name, namelen)); @@ -496,7 +496,7 @@
> COBALT_IMPL(int, shutdown, (int fd, int how))
>         int ret;
>
>         ret = do_ioctl(fd, _RTIOC_SHUTDOWN, (void *)(long)how);
> -       if (ret != -EBADF && ret != -ENOSYS)
> +       if (ret != -ENODEV && ret != -ENOSYS)
>                 return set_errno(ret);
>
>         return __STD(shutdown(fd, how)); @@ -518,7 +518,7 @@
> COBALT_IMPL(void *, mmap64, (void *addr, size_t length, int prot, int flags,
>         rma.flags = flags;
>
>         ret = XENOMAI_SYSCALL3(sc_cobalt_mmap, fd, &rma, &addr);
> -       if (ret != -EBADF && ret != -ENOSYS) {
> +       if (ret != -ENODEV && ret != -ENOSYS) {
>                 ret = set_errno(ret);
>                 if (ret)
>                         return MAP_FAILED;
> --
> 2.21.0
>

________________________________

This message and any attachments are solely for the use of the intended recipients. They may contain privileged and/or confidential information or other information protected from disclosure. If you are not an intended recipient, you are hereby notified that you received this email in error and that any review, dissemination, distribution or copying of this email and any attachment is strictly prohibited. If you have received this email in error, please contact the sender and delete the message and any attachment from your system.

ANDRITZ HYDRO GmbH


Rechtsform/ Legal form: Gesellschaft mit beschränkter Haftung / Corporation

Firmensitz/ Registered seat: Wien

Firmenbuchgericht/ Court of registry: Handelsgericht Wien

Firmenbuchnummer/ Company registration: FN 61833 g

DVR: 0605077

UID-Nr.: ATU14756806


Thank You
________________________________

Re: [PATCH 2/2] cobalt: switch hand over status to -ENODEV for non-RTDM fd

[Philippe Gerum]

On 8/29/19 4:12 PM, Lange Norbert wrote:
> 
> I ran into a rather big issue with linux filehandles
> I use Xenomai master on ipipe-core-4.19.60-x86-5 with those patches,
> (can't be 100% sure its not some kernel/userspace conflict, but I doubt it)
> 
> What happens is that upon a __cobalt_close with a linux filehande, the
> syscall sc_cobalt_close returns EBADF, but that means the libc close will
> never be tried and filehandles are leaking like mad.
> 

You may need v3:

https://lab.xenomai.org/xenomai-rpm.git/commit/?h=for-upstream/next&id=42b7d3d43d9476a101103ad1a3d52ee6822b52e6

-- 
Philippe.

Re: [PATCH 2/2] cobalt: switch hand over status to -ENODEV for non-RTDM fd

[Jan Kiszka]

On 29.08.19 16:12, Lange Norbert via Xenomai wrote:
> 
> I ran into a rather big issue with linux filehandles
> I use Xenomai master on ipipe-core-4.19.60-x86-5 with those patches,
> (can't be 100% sure its not some kernel/userspace conflict, but I doubt it)
> 
> What happens is that upon a __cobalt_close with a linux filehande, the
> syscall sc_cobalt_close returns EBADF, but that means the libc close will
> never be tried and filehandles are leaking like mad.
> 

Ah, good catch. Looks like Philippe's patch was missing a change to rtdm_fd_close().

Thanks a lot for testing pro-actively!

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

RE: [PATCH 2/2] cobalt: switch hand over status to -ENODEV for non-RTDM fd

[Lange Norbert]

> -----Original Message-----
> From: Jan Kiszka <jan.kiszka@siemens.com>
> Sent: Donnerstag, 29. August 2019 16:52
> To: Lange Norbert <norbert.lange@andritz.com>; Philippe Gerum
> <rpm@xenomai.org>; Xenomai (xenomai@xenomai.org)
> <xenomai@xenomai.org>
> Subject: Re: [PATCH 2/2] cobalt: switch hand over status to -ENODEV for non-
> RTDM fd
>
> E-MAIL FROM A NON-ANDRITZ SOURCE: AS A SECURITY MEASURE, PLEASE
> EXERCISE CAUTION WITH E-MAIL CONTENT AND ANY LINKS OR
> ATTACHMENTS.
>
>
> On 29.08.19 16:12, Lange Norbert via Xenomai wrote:
> >
> > I ran into a rather big issue with linux filehandles I use Xenomai
> > master on ipipe-core-4.19.60-x86-5 with those patches, (can't be 100%
> > sure its not some kernel/userspace conflict, but I doubt it)
> >
> > What happens is that upon a __cobalt_close with a linux filehande, the
> > syscall sc_cobalt_close returns EBADF, but that means the libc close
> > will never be tried and filehandles are leaking like mad.
> >
>
> Ah, good catch. Looks like Philippe's patch was missing a change to
> rtdm_fd_close().

Yes, but his v3 works.

> Thanks a lot for testing pro-actively!

You are welcome, its less benign than you might think though,
Philippe's patches (allow for device teardown) were requested from myself.

How does Xenomai/cobalt handle kernel/userspace conflicts like these BTW,
Is there some ABI variable that needs to be incremented and can detect mismatches?
(if you use an old libcobalt on a new kernel module with patchset or vice verse, it would result in leaks or other issues)

Kind regards,
Norbert
________________________________

This message and any attachments are solely for the use of the intended recipients. They may contain privileged and/or confidential information or other information protected from disclosure. If you are not an intended recipient, you are hereby notified that you received this email in error and that any review, dissemination, distribution or copying of this email and any attachment is strictly prohibited. If you have received this email in error, please contact the sender and delete the message and any attachment from your system.

ANDRITZ HYDRO GmbH


Rechtsform/ Legal form: Gesellschaft mit beschränkter Haftung / Corporation

Firmensitz/ Registered seat: Wien

Firmenbuchgericht/ Court of registry: Handelsgericht Wien

Firmenbuchnummer/ Company registration: FN 61833 g

DVR: 0605077

UID-Nr.: ATU14756806


Thank You
________________________________

Re: [PATCH 2/2] cobalt: switch hand over status to -ENODEV for non-RTDM fd

[Jan Kiszka]

On 30.08.19 11:58, Lange Norbert wrote:
> 
> 
>> -----Original Message-----
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>> Sent: Donnerstag, 29. August 2019 16:52
>> To: Lange Norbert <norbert.lange@andritz.com>; Philippe Gerum
>> <rpm@xenomai.org>; Xenomai (xenomai@xenomai.org)
>> <xenomai@xenomai.org>
>> Subject: Re: [PATCH 2/2] cobalt: switch hand over status to -ENODEV for non-
>> RTDM fd
>>
>> E-MAIL FROM A NON-ANDRITZ SOURCE: AS A SECURITY MEASURE, PLEASE
>> EXERCISE CAUTION WITH E-MAIL CONTENT AND ANY LINKS OR
>> ATTACHMENTS.
>>
>>
>> On 29.08.19 16:12, Lange Norbert via Xenomai wrote:
>>>
>>> I ran into a rather big issue with linux filehandles I use Xenomai
>>> master on ipipe-core-4.19.60-x86-5 with those patches, (can't be 100%
>>> sure its not some kernel/userspace conflict, but I doubt it)
>>>
>>> What happens is that upon a __cobalt_close with a linux filehande, the
>>> syscall sc_cobalt_close returns EBADF, but that means the libc close
>>> will never be tried and filehandles are leaking like mad.
>>>
>>
>> Ah, good catch. Looks like Philippe's patch was missing a change to
>> rtdm_fd_close().
> 
> Yes, but his v3 works.
> 
>> Thanks a lot for testing pro-actively!
> 
> You are welcome, its less benign than you might think though,
> Philippe's patches (allow for device teardown) were requested from myself.
> 
> How does Xenomai/cobalt handle kernel/userspace conflicts like these BTW,
> Is there some ABI variable that needs to be incremented and can detect mismatches?
> (if you use an old libcobalt on a new kernel module with patchset or vice verse, it would result in leaks or other issues)

There is an ABI revision check between kernel and userland, and there are 
feature checks. We will likely need an ABI bump here, which did not take place 
in master so far (compared to 3.0). Follow XENOMAI_ABI_REV on that.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux