Re: [PATCH v1 10/10] j1939: socket: make sure all sessions are finished on close

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Marc,

On 12/12/18 3:57 PM, Marc Kleine-Budde wrote:
On 12/5/18 7:07 AM, Oleksij Rempel wrote:
Currently, (E)TP transfers will be aborted, as soon application will
call close() or exit, as the socket will be automatically closed by the
kernel.

With this patch, the application hangs on the close() call until all
data has been send out. This might take a while. What happens if you
attach a debugger? I think the wait_event_interruptible() will return as
attaching a debugger involves signals.

I'm not sure whether this could be solved differently.

It might be that we're currently running into a similar problem with isotp.c, see

	https://github.com/linux-can/can-utils/issues/113

But I think UDP/IP should have an analogue requirement. Will take a look into it.

Regards,
Oliver


Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
---
  net/can/j1939/socket.c | 10 ++++++++++
  1 file changed, 10 insertions(+)

diff --git a/net/can/j1939/socket.c b/net/can/j1939/socket.c
index d6332483350f..e003d3cae24c 100644
--- a/net/can/j1939/socket.c
+++ b/net/can/j1939/socket.c
@@ -101,6 +101,13 @@ static inline void j1939_sock_pending_add(struct sock *sk)
  	atomic_inc(&jsk->skb_pending);
  }
+static int j1939_sock_pending_get(struct sock *sk)
+{
+	struct j1939_sock *jsk = j1939_sk(sk);
+
+	return atomic_read(&jsk->skb_pending);
+}
+
  void j1939_sock_pending_del(struct sock *sk)
  {
  	struct j1939_sock *jsk = j1939_sk(sk);
@@ -404,6 +411,9 @@ static int j1939_sk_release(struct socket *sock)
  	if (jsk->state & J1939_SOCK_BOUND) {
  		struct net_device *ndev;
+ wait_event_interruptible(jsk->waitq,
+					 j1939_sock_pending_get(&jsk->sk) == 0);

If a signal is received, wait_event will return -ERESTARTSYS.

However EINTR is the currect return value close():

EINTR  The close() call was interrupted by a signal; see signal(7).

On Linux the filedescriptor is closed regardless of a signal or not:

The EINTR error is a somewhat special case.  Regarding the
EINTR error, POSIX.1-2013 says:

If close() is interrupted by a signal that is to be
caught, it shall return -1 with errno set to EINTR and the state of
fildes is unspecified.

This  permits the behavior that occurs on Linux and many other
implementations, where, as with other errors that may be reported by
close(), the file descriptor is guaranteed to be closed.  However, it
also permits another possibility: that the implementation returns an
EINTR error and keeps the file descriptor open.  (According to its
documentation, HP-UX's close() does this.)  The caller must then once
more use close() to close the file descriptor, to avoid file
descriptor leaks.  This  divergence  in  implementation  behaviors
provides  a  difficult  hurdle for portable applications, since on
many implementations, close() must not be called again after an EINTR
error, and on at least one, close() must be called again.  There are
plans to address this conundrum for the next major release of the
POSIX.1 standard.

So we have to check if the correct return value is received by the
close() call in userspace in case of a signal delivery.

+
  		spin_lock_bh(&j1939_socks_lock);
  		list_del_init(&jsk->list);
  		spin_unlock_bh(&j1939_socks_lock);


Marc




[Index of Archives]     [Automotive Discussions]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [CAN Bus]

  Powered by Linux