| Message ID | 20210122150638.210444-1-willy@infradead.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | af_unix: Allow Unix sockets to raise SIGURG | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | warning | 7 maintainers not CCed: gustavoars@kernel.org jingxiangfeng@huawei.com pabeni@redhat.com orcohen2006@gmail.com tklauser@distanz.ch cai@lca.pw ktkhai@virtuozzo.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 4 this patch: 4 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 17 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 4 this patch: 4 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Fri, 22 Jan 2021 15:06:37 +0000 Matthew Wilcox (Oracle) wrote: > From: Rao Shoaib <rao.shoaib@oracle.com> > > 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? > 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 <rao.shoaib@oracle.com> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > 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;
On 1/25/21 3:36 PM, Jakub Kicinski wrote: > On Fri, 22 Jan 2021 15:06:37 +0000 Matthew Wilcox (Oracle) wrote: >> From: Rao Shoaib <rao.shoaib@oracle.com> >> >> 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 <rao.shoaib@oracle.com> >> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> >> --- >> 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;
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 +0000 Matthew Wilcox (Oracle) wrote: > >> From: Rao Shoaib <rao.shoaib@oracle.com> > >> > >> 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. 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. > >> 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 <rao.shoaib@oracle.com> > >> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > >> --- > >> 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;
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 +0000 Matthew Wilcox (Oracle) wrote: > > > From: Rao Shoaib <rao.shoaib@oracle.com> > > > > > > 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. 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. 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. 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.
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 +0000 Matthew Wilcox (Oracle) wrote: >>>> From: Rao Shoaib <rao.shoaib@oracle.com> >>>> >>>> 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 <rao.shoaib@oracle.com> >>>> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> >>>> --- >>>> 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;
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 +0000 Matthew Wilcox (Oracle) wrote: >>>> From: Rao Shoaib <rao.shoaib@oracle.com> >>>> >>>> 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 >
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 +0000 Matthew Wilcox (Oracle) wrote: >>>>> From: Rao Shoaib <rao.shoaib@oracle.com> >>>>> >>>>> 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 > >>
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. > 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.
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.
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. So you're hijacking the MSG_OOB to send a signal, because OOB also sends a signal. But there is nothing OOB about the data itself. So I'm asking you to make sure that there is no data in the message. That way when someone wants _actual_ OOB data on UNIX sockets they can implement it without breaking backwards compatibility of the kernel uAPI. > 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.
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.
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.
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. > 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 :)
On Fri, Jan 29, 2021 at 01:18:20PM -0800, 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. I'd encourage anyone thinking about "using OOB" to read https://tools.ietf.org/html/rfc6093 first. Basically, TCP does not actually provide an OOB mechanism, and frankly Unix sockets shouldn't try either. As an aside, we should probably remove the net.ipv4.tcp_stdurg sysctl since it's broken. Some operating systems provide a system-wide toggle to override this behavior and interpret the semantics of the Urgent Pointer as clarified in RFC 1122. However, this system-wide toggle has been found to be inconsistent. For example, Linux provides the sysctl "tcp_stdurg" (i.e., net.ipv4.tcp_stdurg) that, when set, supposedly changes the system behavior to interpret the semantics of the TCP Urgent Pointer as specified in RFC 1122. However, this sysctl changes the semantics of the Urgent Pointer only for incoming segments (i.e., not for outgoing segments). This means that if this sysctl is set, an application might be unable to interoperate with itself if both the TCP sender and the TCP receiver are running on the same host. > > 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 :) If we were going to do something different from TCP sockets to generate a remote SIGURG, then it would ideally be an entirely different mechanism (eg a fcntl()) that could also be implemented by pipes. But I think it's worth just saying "MSG_OOB on Unix sockets generates a signal on the remote end, just like it does on TCP sockets. Unix sockets do not actually support OOB data and behave like TCP sockets with SO_OOBINLINE set as recommended in RFC 6093".
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 :)
> I'd encourage anyone thinking about "using OOB" to read > https://tools.ietf.org/html/rfc6093 first. Basically, TCP does not > actually provide an OOB mechanism, and frankly Unix sockets shouldn't > try either. OOB data maps much better onto ISO transport 'expedited data' than anything in a bytestream protocol like TCP. There you can send a message (it is message oriented) that isn't subject to normal data flow control. The length is limited (IIRC 32 bytes) and expedited data has its own credit of one, but can overtake (and is expected to overtake) flow control blocked normal data. All TCP provides is a byte sequence number for OOB data. This is just a marker in the bytestream. It really doesn't map onto the socket OOB data data all. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
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;