Re: [PATCH] af_unix: Allow Unix sockets to raise SIGURG
On 1/29/21 1:18 PM, Jakub Kicinski wrote: On Fri, 29 Jan 2021 12:44:44 -0800 Shoaib Rao wrote: On 1/29/21 12:18 PM, Jakub Kicinski wrote: On Fri, 29 Jan 2021 12:10:21 -0800 Shoaib Rao wrote: The code does not care about the size of data -- All it does is that if MSG_OOB is set it will deliver the signal to the peer process irrespective of the length of the data (which can be zero length). Let's look at the code of unix_stream_sendmsg() It does the following (sent is initialized to zero) Okay. Let me try again. AFAICS your code makes it so that data sent with MSG_OOB is treated like any other data. It just sends a signal. Correct. So you're hijacking the MSG_OOB to send a signal, because OOB also sends a signal. Correct. But there is nothing OOB about the data itself. Correct. So I'm asking you to make sure that there is no data in the message. Yes I can do that. That way when someone wants _actual_ OOB data on UNIX sockets they can implement it without breaking backwards compatibility of the kernel uAPI. I see what you are trying to achieve. However it may not work. Let's assume that __actual__ OOB data has been implemented. An application sends a zero length message with MSG_OOB, after that it sends some data (not suppose to be OOB data). How is the receiver going to differentiate if the data an OOB or not. THB I've never written any application which would use OOB, so in practice IDK. But from kernel code and looking at man pages when OOBINLINE is not set for OOB data to be received MSG_OOB has to be set in the recv syscall. Thinking a little more about your suggestion, I think it can work but the application will have to do some more work to differentiate. I would prefer it would not have to. Anyways, I will re-submit the patch with the zero length check. Thanks a lot for your comments, Shoaib We could use a different flag (MSG_SIGURG) or implement the _actual_ OOB data semantics (If anyone is interested in it). MSG_SIGURG could be a generic flag that just sends SIGURG irrespective of the length of the data. No idea on the SIGURG parts :)
Re: [PATCH] af_unix: Allow Unix sockets to raise SIGURG
On 1/29/21 12:44 PM, Shoaib Rao wrote:
On 1/29/21 12:18 PM, Jakub Kicinski wrote:
On Fri, 29 Jan 2021 12:10:21 -0800 Shoaib Rao wrote:
On 1/29/21 12:02 PM, Jakub Kicinski wrote:
On Fri, 29 Jan 2021 11:48:15 -0800 Shoaib Rao wrote:
Data was discarded because the flag was not supported, this patch
changes that but does not support any urgent data.
When you say it does not support any urgent data do you mean the
message len must be == 0 because something is checking it, or that
the code does not support its handling?
I'm perfectly fine with the former, just point me at the check,
please.
The code does not care about the size of data -- All it does is that if
MSG_OOB is set it will deliver the signal to the peer process
irrespective of the length of the data (which can be zero length).
Let's
look at the code of unix_stream_sendmsg() It does the following
(sent is
initialized to zero)
Okay. Let me try again. AFAICS your code makes it so that data sent
with MSG_OOB is treated like any other data. It just sends a signal.
Correct.
So you're hijacking the MSG_OOB to send a signal, because OOB also
sends a signal.
Correct.
But there is nothing OOB about the data itself.
Correct.
So
I'm asking you to make sure that there is no data in the message.
Yes I can do that.
That way when someone wants _actual_ OOB data on UNIX sockets they
can implement it without breaking backwards compatibility of the
kernel uAPI.
I see what you are trying to achieve. However it may not work.
Let's assume that __actual__ OOB data has been implemented. An
application sends a zero length message with MSG_OOB, after that it
sends some data (not suppose to be OOB data). How is the receiver
going to differentiate if the data an OOB or not.
We could use a different flag (MSG_SIGURG) or implement the _actual_
OOB data semantics (If anyone is interested in it). MSG_SIGURG could
be a generic flag that just sends SIGURG irrespective of the length of
the data.
Shoaib
There is a relevant issue that I want to point out, Is it acceptable to
send SIGURG without the receiver having any means to know what the
urgent condition is?
Shoaib
while (sent < len) {
size = len - sent;
<..>
}
if (msg->msg_flags & MSG_OOB)
sk_send_sigurg(other);
Before the patch there was a check above the while loop that checked
the
flag and returned and error, that has been removed.
Re: [PATCH] af_unix: Allow Unix sockets to raise SIGURG
On 1/29/21 12:18 PM, Jakub Kicinski wrote:
On Fri, 29 Jan 2021 12:10:21 -0800 Shoaib Rao wrote:
On 1/29/21 12:02 PM, Jakub Kicinski wrote:
On Fri, 29 Jan 2021 11:48:15 -0800 Shoaib Rao wrote:
Data was discarded because the flag was not supported, this patch
changes that but does not support any urgent data.
When you say it does not support any urgent data do you mean the
message len must be == 0 because something is checking it, or that
the code does not support its handling?
I'm perfectly fine with the former, just point me at the check, please.
The code does not care about the size of data -- All it does is that if
MSG_OOB is set it will deliver the signal to the peer process
irrespective of the length of the data (which can be zero length). Let's
look at the code of unix_stream_sendmsg() It does the following (sent is
initialized to zero)
Okay. Let me try again. AFAICS your code makes it so that data sent
with MSG_OOB is treated like any other data. It just sends a signal.
Correct.
So you're hijacking the MSG_OOB to send a signal, because OOB also
sends a signal.
Correct.
But there is nothing OOB about the data itself.
Correct.
So
I'm asking you to make sure that there is no data in the message.
Yes I can do that.
That way when someone wants _actual_ OOB data on UNIX sockets they
can implement it without breaking backwards compatibility of the
kernel uAPI.
I see what you are trying to achieve. However it may not work.
Let's assume that __actual__ OOB data has been implemented. An
application sends a zero length message with MSG_OOB, after that it
sends some data (not suppose to be OOB data). How is the receiver going
to differentiate if the data an OOB or not.
We could use a different flag (MSG_SIGURG) or implement the _actual_ OOB
data semantics (If anyone is interested in it). MSG_SIGURG could be a
generic flag that just sends SIGURG irrespective of the length of the data.
Shoaib
while (sent < len) {
size = len - sent;
<..>
}
if (msg->msg_flags & MSG_OOB)
sk_send_sigurg(other);
Before the patch there was a check above the while loop that checked the
flag and returned and error, that has been removed.
Re: [PATCH] af_unix: Allow Unix sockets to raise SIGURG
On 1/29/21 12:02 PM, Jakub Kicinski wrote:
On Fri, 29 Jan 2021 11:48:15 -0800 Shoaib Rao wrote:
SO_OOBINLINE does not control the delivery of signal, It controls how
OOB Byte is delivered. It may not be obvious but this change does not
deliver any Byte, just a signal. So, as long as sendmsg flag contains
MSG_OOB, signal will be delivered just like it happens for TCP.
Not as far as I can read this code. If MSG_OOB is set the data from the
message used to be discarded, and EOPNOTSUPP returned. Now the data gets
queued to the socket, and will be read inline.
Data was discarded because the flag was not supported, this patch
changes that but does not support any urgent data.
When you say it does not support any urgent data do you mean the
message len must be == 0 because something is checking it, or that
the code does not support its handling?
I'm perfectly fine with the former, just point me at the check, please.
The code does not care about the size of data -- All it does is that if
MSG_OOB is set it will deliver the signal to the peer process
irrespective of the length of the data (which can be zero length). Let's
look at the code of unix_stream_sendmsg() It does the following (sent is
initialized to zero)
while (sent < len) {
size = len - sent;
<..>
}
if (msg->msg_flags & MSG_OOB)
sk_send_sigurg(other);
Before the patch there was a check above the while loop that checked the
flag and returned and error, that has been removed.
Shoaib
OOB data has some semantics that would have to be followed and if we
support SO_OOBINLINE we would have to support NOT SO_OOBINLINE.
One can argue that we add a socket option to allow this OR just do what
TCP does.
Re: [PATCH] af_unix: Allow Unix sockets to raise SIGURG
On 1/29/21 11:54 AM, Shoaib Rao wrote: On 1/29/21 11:19 AM, Matthew Wilcox wrote: On Fri, Jan 29, 2021 at 09:56:48AM -0800, Shoaib Rao wrote: On 1/25/21 3:36 PM, Jakub Kicinski wrote: On Fri, 22 Jan 2021 15:06:37 + Matthew Wilcox (Oracle) wrote: From: Rao Shoaib TCP sockets allow SIGURG to be sent to the process holding the other end of the socket. Extend Unix sockets to have the same ability. The API is the same in that the sender uses sendmsg() with MSG_OOB to raise SIGURG. Unix sockets behave in the same way as TCP sockets with SO_OOBINLINE set. Noob question, if we only want to support the inline mode, why don't we require SO_OOBINLINE to have been called on @other? Wouldn't that provide more consistent behavior across address families? With the current implementation the receiver will also not see MSG_OOB set in msg->msg_flags, right? SO_OOBINLINE does not control the delivery of signal, It controls how OOB Byte is delivered. It may not be obvious but this change does not deliver any Byte, just a signal. So, as long as sendmsg flag contains MSG_OOB, signal will be delivered just like it happens for TCP. I don't think that's the question Jakub is asking. As I understand it, if you send a MSG_OOB on a TCP socket and the receiver calls recvmsg(), it will see MSG_OOB set, even if SO_OOBINLINE is set. No it wont. Application just gets a signal. That wouldn't happen with Unix sockets. I'm OK with that difference in behaviour, because MSG_OOB on Unix sockets _is not_ for sending out of band data. It's just for sending an urgent signal. That is what I just explained in my email As you say, MSG_OOB does not require data to be sent for unix sockets (unlike TCP which always requires at least one byte), but one can choose to send data as part of a message which has MSG_OOB set. It won't be tagged in any special way. To Jakub's other question, we could require SO_OOBINLINE to be set. That'd provide another layer of insurance against applications being surprised by a SIGURG they weren't expecting. I don't know that it's really worth it though. SO_OOBINLINE has a meaning, that the urgent byte is part of the stream and using SO_OOBLINE to allow signalling would be wrong/confusing. We could add a socket option on the receiver to indicate if it supports or wants the signal. One thing I wasn't clear about, and maybe you know, if we send a MSG_OOB, does this patch cause this part of the tcp(7) manpage to be true for unix sockets too? When out-of-band data is present, select(2) indicates the file descrip‐ tor as having an exceptional condition and poll (2) indicates a POLLPRI event. No because there is no data involved. Poll is associated with data not signals. Shoaib SO_OOBINLINE implies there is urgent data inline -- This patch will send a signal even if there is no data. Shoaib
Re: [PATCH] af_unix: Allow Unix sockets to raise SIGURG
On 1/29/21 11:19 AM, Matthew Wilcox wrote: On Fri, Jan 29, 2021 at 09:56:48AM -0800, Shoaib Rao wrote: On 1/25/21 3:36 PM, Jakub Kicinski wrote: On Fri, 22 Jan 2021 15:06:37 + Matthew Wilcox (Oracle) wrote: From: Rao Shoaib TCP sockets allow SIGURG to be sent to the process holding the other end of the socket. Extend Unix sockets to have the same ability. The API is the same in that the sender uses sendmsg() with MSG_OOB to raise SIGURG. Unix sockets behave in the same way as TCP sockets with SO_OOBINLINE set. Noob question, if we only want to support the inline mode, why don't we require SO_OOBINLINE to have been called on @other? Wouldn't that provide more consistent behavior across address families? With the current implementation the receiver will also not see MSG_OOB set in msg->msg_flags, right? SO_OOBINLINE does not control the delivery of signal, It controls how OOB Byte is delivered. It may not be obvious but this change does not deliver any Byte, just a signal. So, as long as sendmsg flag contains MSG_OOB, signal will be delivered just like it happens for TCP. I don't think that's the question Jakub is asking. As I understand it, if you send a MSG_OOB on a TCP socket and the receiver calls recvmsg(), it will see MSG_OOB set, even if SO_OOBINLINE is set. No it wont. Application just gets a signal. That wouldn't happen with Unix sockets. I'm OK with that difference in behaviour, because MSG_OOB on Unix sockets _is not_ for sending out of band data. It's just for sending an urgent signal. That is what I just explained in my email As you say, MSG_OOB does not require data to be sent for unix sockets (unlike TCP which always requires at least one byte), but one can choose to send data as part of a message which has MSG_OOB set. It won't be tagged in any special way. To Jakub's other question, we could require SO_OOBINLINE to be set. That'd provide another layer of insurance against applications being surprised by a SIGURG they weren't expecting. I don't know that it's really worth it though. SO_OOBINLINE has a meaning, that the urgent byte is part of the stream and using SO_OOBLINE to allow signalling would be wrong/confusing. We could add a socket option on the receiver to indicate if it supports or wants the signal. One thing I wasn't clear about, and maybe you know, if we send a MSG_OOB, does this patch cause this part of the tcp(7) manpage to be true for unix sockets too? When out-of-band data is present, select(2) indicates the file descrip‐ tor as having an exceptional condition and poll (2) indicates a POLLPRI event. No because there is no data involved. Poll is associated with data not signals. Shoaib
Re: [PATCH] af_unix: Allow Unix sockets to raise SIGURG
On 1/29/21 11:06 AM, Jakub Kicinski wrote:
On Fri, 29 Jan 2021 09:56:48 -0800 Shoaib Rao wrote:
On 1/25/21 3:36 PM, Jakub Kicinski wrote:
On Fri, 22 Jan 2021 15:06:37 + Matthew Wilcox (Oracle) wrote:
From: Rao Shoaib
TCP sockets allow SIGURG to be sent to the process holding the other
end of the socket. Extend Unix sockets to have the same ability.
The API is the same in that the sender uses sendmsg() with MSG_OOB to
raise SIGURG. Unix sockets behave in the same way as TCP sockets with
SO_OOBINLINE set.
Noob question, if we only want to support the inline mode, why don't we
require SO_OOBINLINE to have been called on @other? Wouldn't that
provide more consistent behavior across address families?
With the current implementation the receiver will also not see MSG_OOB
set in msg->msg_flags, right?
SO_OOBINLINE does not control the delivery of signal, It controls how
OOB Byte is delivered. It may not be obvious but this change does not
deliver any Byte, just a signal. So, as long as sendmsg flag contains
MSG_OOB, signal will be delivered just like it happens for TCP.
Not as far as I can read this code. If MSG_OOB is set the data from the
message used to be discarded, and EOPNOTSUPP returned. Now the data gets
queued to the socket, and will be read inline.
Data was discarded because the flag was not supported, this patch
changes that but does not support any urgent data.
OOB data has some semantics that would have to be followed and if we
support SO_OOBINLINE we would have to support NOT SO_OOBINLINE.
One can argue that we add a socket option to allow this OR just do what
TCP does.
Shoaib
Sure, you also add firing of the signal, which is fine. The removal of
the error check is the code I'm pointing at, so to speak.
That is the change in behavior that this change is making.
SIGURG is ignored by default, so applications which do not know about this
feature will be unaffected. In addition to installing a SIGURG handler,
the receiving application must call F_SETOWN or F_SETOWN_EX to indicate
which process or thread should receive the signal.
Signed-off-by: Rao Shoaib
Signed-off-by: Matthew Wilcox (Oracle)
---
net/unix/af_unix.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 41c3303c3357..849dff688c2c 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1837,8 +1837,6 @@ static int unix_stream_sendmsg(struct socket *sock,
struct msghdr *msg,
return err;
err = -EOPNOTSUPP;
- if (msg->msg_flags&MSG_OOB)
- goto out_err;
if (msg->msg_namelen) {
err = sk->sk_state == TCP_ESTABLISHED ? -EISCONN : -EOPNOTSUPP;
@@ -1903,6 +1901,9 @@ static int unix_stream_sendmsg(struct socket *sock,
struct msghdr *msg,
sent += size;
}
+ if (msg->msg_flags & MSG_OOB)
+ sk_send_sigurg(other);
+
scm_destroy(&scm);
return sent;
Re: [PATCH] af_unix: Allow Unix sockets to raise SIGURG
On 1/25/21 3:36 PM, Jakub Kicinski wrote:
On Fri, 22 Jan 2021 15:06:37 + Matthew Wilcox (Oracle) wrote:
From: Rao Shoaib
TCP sockets allow SIGURG to be sent to the process holding the other
end of the socket. Extend Unix sockets to have the same ability.
The API is the same in that the sender uses sendmsg() with MSG_OOB to
raise SIGURG. Unix sockets behave in the same way as TCP sockets with
SO_OOBINLINE set.
Noob question, if we only want to support the inline mode, why don't we
require SO_OOBINLINE to have been called on @other? Wouldn't that
provide more consistent behavior across address families?
With the current implementation the receiver will also not see MSG_OOB
set in msg->msg_flags, right?
SO_OOBINLINE does not control the delivery of signal, It controls how OOB Byte
is delivered. It may not be obvious but this change does not deliver any Byte,
just a signal. So, as long as sendmsg flag contains MSG_OOB, signal will be
delivered just like it happens for TCP.
Shoaib
SIGURG is ignored by default, so applications which do not know about this
feature will be unaffected. In addition to installing a SIGURG handler,
the receiving application must call F_SETOWN or F_SETOWN_EX to indicate
which process or thread should receive the signal.
Signed-off-by: Rao Shoaib
Signed-off-by: Matthew Wilcox (Oracle)
---
net/unix/af_unix.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 41c3303c3357..849dff688c2c 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1837,8 +1837,6 @@ static int unix_stream_sendmsg(struct socket *sock,
struct msghdr *msg,
return err;
err = -EOPNOTSUPP;
- if (msg->msg_flags&MSG_OOB)
- goto out_err;
if (msg->msg_namelen) {
err = sk->sk_state == TCP_ESTABLISHED ? -EISCONN : -EOPNOTSUPP;
@@ -1903,6 +1901,9 @@ static int unix_stream_sendmsg(struct socket *sock,
struct msghdr *msg,
sent += size;
}
+ if (msg->msg_flags & MSG_OOB)
+ sk_send_sigurg(other);
+
scm_destroy(&scm);
return sent;
