diff mbox series

af_unix: Allow Unix sockets to raise SIGURG

Message ID 20210122150638.210444-1-willy@infradead.org
State New
Headers show
Series af_unix: Allow Unix sockets to raise SIGURG | expand

Commit Message

Matthew Wilcox Jan. 22, 2021, 3:06 p.m. UTC
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.

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(-)

Comments

Jakub Kicinski Jan. 25, 2021, 11:36 p.m. UTC | #1
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;
Rao Shoaib Jan. 29, 2021, 5:56 p.m. UTC | #2
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;
Jakub Kicinski Jan. 29, 2021, 7:06 p.m. UTC | #3
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;
Matthew Wilcox Jan. 29, 2021, 7:19 p.m. UTC | #4
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.
Rao Shoaib Jan. 29, 2021, 7:48 p.m. UTC | #5
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;
Rao Shoaib Jan. 29, 2021, 7:54 p.m. UTC | #6
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

>
Rao Shoaib Jan. 29, 2021, 8:01 p.m. UTC | #7
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

>

>>
Jakub Kicinski Jan. 29, 2021, 8:02 p.m. UTC | #8
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.
Rao Shoaib Jan. 29, 2021, 8:10 p.m. UTC | #9
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.
Jakub Kicinski Jan. 29, 2021, 8:18 p.m. UTC | #10
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.
Rao Shoaib Jan. 29, 2021, 8:44 p.m. UTC | #11
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.
Rao Shoaib Jan. 29, 2021, 8:49 p.m. UTC | #12
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.
Jakub Kicinski Jan. 29, 2021, 9:18 p.m. UTC | #13
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 :)
Matthew Wilcox Jan. 29, 2021, 9:32 p.m. UTC | #14
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".
Rao Shoaib Jan. 29, 2021, 9:54 p.m. UTC | #15
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 :)
David Laight Jan. 29, 2021, 11:47 p.m. UTC | #16
> 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 mbox series

Patch

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;