Re: [patch net-next 0/2] Fixes for raw diag sockets handling

[David Miller]

From: Cyrill Gorcunov 
Date: Wed, 02 Nov 2016 15:36:30 +0300

> Hi! Here are a few fixes for raw-diag sockets handling: missing
> sock_put call and jump for exiting from nested cycle. I made
> patches for iproute2 as well so will send them out soon.

Series applied, thanks.

Re: [patch net-next 0/2] Fixes for raw diag sockets handling

[Cyrill Gorcunov]

On Wed, Nov 02, 2016 at 09:36:55AM -0600, David Ahern wrote:
> 
> Limited to raw sockets or are you looking at multiple spec options (dev, 
> address, port)?
> 
> I have not seen issues with tcp or udp. Running:
> 
> ss -aK 'dev == red' 
> 
> drops all sockets bound to device 'red' (or at least signaling the socket 
> failure for the app to handle):

Limited to raw socket. I didn't modify lookup kernel code but use already 
existing helpers.
The tcp/udp sockets do use port value in lookup (iirc, don't have code under my 
hand
at moment), in turn raw lookup uses only net,raw-protocol, src/dst and device 
index.
In my test case the sokets were unconnected so the have no address but bound to
device and I hit mismatch. Then looking into inet matching code I found this 
weird
snippet I posted previously.

> 
> root@jessie4:~# ss -ap 'dev == red'
> Netid  State  Recv-Q Send-Q Local Address:Port  
> Peer Address:Port
> udpUNCONN 0  0  *%red:12345   
>  *:* users:(("vrf-test",pid=765,fd=3))
> tcpLISTEN 0  1  *%red:12345   
>  *:* users:(("vrf-test",pid=766,fd=3))
> tcpESTAB  0  0 10.100.1.4%red:ssh   
> 10.100.1.254:60298 users:(("sshd",pid=738,fd=3))
> 
> root@jessie4:~# ss -aKp 'dev == red'
> Netid State  Recv-Q Send-Q  Local Address:Port
>Peer Address:Port
> udp   UNCONN 0  0   *%red:12345   
>   *:* 
> users:(("vrf-test",pid=765,fd=3))
> tcp   LISTEN 0  1   *%red:12345   
>   *:* 
> users:(("vrf-test",pid=766,fd=3))
> tcp   ESTAB  0  0  10.100.1.4%red:ssh 
>10.100.1.254:60298 
> users:(("sshd",pid=738,fd=3))
> 
> root@jessie4:~# ss -ap 'dev == red'
> Netid State  Recv-Q Send-Q  Local Address:Port
>Peer Address:Port

Cyrill

Re: [patch net-next 0/2] Fixes for raw diag sockets handling

[David Ahern]

On 11/2/16 9:29 AM, Cyrill Gorcunov wrote:
> On Wed, Nov 02, 2016 at 09:10:32AM -0600, David Ahern wrote:
>>> @__dif != 0 the match may return socket where sk_bound_dev_if = 0
>>> instead of completely matching one. Isn't it?
>>
>> yes. I recently added an exact_dif to the lookup for listener sockets
>> (see compute_score). Something like that could be added to INET_MATCH.
> 
> Seem so. I need to revisit this moment. Because with current lookup code
> iproute2 patches I made and been testing do not kill all sockets bound
> to particular device in one pass (because request from userspace asks
> for index 15 in my case but kernel return one with index 0). At first
> I thought I made a mistake in userspace code but once I added printk's
> into kernel I found that here some strange results over lookup.

Limited to raw sockets or are you looking at multiple spec options (dev, 
address, port)?

I have not seen issues with tcp or udp. Running:

ss -aK 'dev == red' 

drops all sockets bound to device 'red' (or at least signaling the socket 
failure for the app to handle):

root@jessie4:~# ss -ap 'dev == red'
Netid  State  Recv-Q Send-Q Local Address:Port  
Peer Address:Port
udpUNCONN 0  0  *%red:12345 
   *:* users:(("vrf-test",pid=765,fd=3))
tcpLISTEN 0  1  *%red:12345 
   *:* users:(("vrf-test",pid=766,fd=3))
tcpESTAB  0  0 10.100.1.4%red:ssh   
10.100.1.254:60298 users:(("sshd",pid=738,fd=3))

root@jessie4:~# ss -aKp 'dev == red'
Netid State  Recv-Q Send-Q  Local Address:Port  
 Peer Address:Port
udp   UNCONN 0  0   *%red:12345 
*:* 
users:(("vrf-test",pid=765,fd=3))
tcp   LISTEN 0  1   *%red:12345 
*:* 
users:(("vrf-test",pid=766,fd=3))
tcp   ESTAB  0  0  10.100.1.4%red:ssh   
 10.100.1.254:60298 
users:(("sshd",pid=738,fd=3))

root@jessie4:~# ss -ap 'dev == red'
Netid State  Recv-Q Send-Q  Local Address:Port  
 Peer Address:Port

Re: [patch net-next 0/2] Fixes for raw diag sockets handling

[Cyrill Gorcunov]

On Wed, Nov 02, 2016 at 09:10:32AM -0600, David Ahern wrote:
> > @__dif != 0 the match may return socket where sk_bound_dev_if = 0
> > instead of completely matching one. Isn't it?
> 
> yes. I recently added an exact_dif to the lookup for listener sockets
> (see compute_score). Something like that could be added to INET_MATCH.

Seem so. I need to revisit this moment. Because with current lookup code
iproute2 patches I made and been testing do not kill all sockets bound
to particular device in one pass (because request from userspace asks
for index 15 in my case but kernel return one with index 0). At first
I thought I made a mistake in userspace code but once I added printk's
into kernel I found that here some strange results over lookup.

Cyrill

Re: [patch net-next 0/2] Fixes for raw diag sockets handling

[David Ahern]

On 11/2/16 6:36 AM, Cyrill Gorcunov wrote:
> Also I have a question about sockets lookup not for raw diag only
> (though I didn't modify lookup procedure) but in general: the structure
> inet_diag_req_v2 has inet_diag_sockid::idiag_if member which supposed to
> carry interface index from userspace request.
> 
> Then for example in INET_MATCH (include/net/inet_hashtables.h),
> the __dif parameter (which is @idiag_if) compared with @sk_bound_dev_if
> *iif* the sk_bound_dev_if has been ever set. Thus if say someone
> looks up for paticular device with specified index if the
> rest of parameters match and SO_BINDTODEVICE never been called
> for this device we return the socket even if idiag_if is not zero.
> Is it supposed to be so? Or I miss something obvious?
> 
> I mean this snippet
> 
> 
>(!(__sk)->sk_bound_dev_if  ||  \
>  ((__sk)->sk_bound_dev_if == (__dif)))&&  \
> 
> when someone calls for destory sockets on particular interface and
> @__dif != 0 the match may return socket where sk_bound_dev_if = 0
> instead of completely matching one. Isn't it?

yes. I recently added an exact_dif to the lookup for listener sockets (see 
compute_score). Something like that could be added to INET_MATCH.

[patch net-next 0/2] Fixes for raw diag sockets handling

[Cyrill Gorcunov]

Hi! Here are a few fixes for raw-diag sockets handling: missing
sock_put call and jump for exiting from nested cycle. I made
patches for iproute2 as well so will send them out soon.

Also I have a question about sockets lookup not for raw diag only
(though I didn't modify lookup procedure) but in general: the structure
inet_diag_req_v2 has inet_diag_sockid::idiag_if member which supposed to
carry interface index from userspace request.

Then for example in INET_MATCH (include/net/inet_hashtables.h),
the __dif parameter (which is @idiag_if) compared with @sk_bound_dev_if
*iif* the sk_bound_dev_if has been ever set. Thus if say someone
looks up for paticular device with specified index if the
rest of parameters match and SO_BINDTODEVICE never been called
for this device we return the socket even if idiag_if is not zero.
Is it supposed to be so? Or I miss something obvious?

I mean this snippet


 (!(__sk)->sk_bound_dev_if  ||  \
   ((__sk)->sk_bound_dev_if == (__dif)))&&  \

when someone calls for destory sockets on particular interface and
@__dif != 0 the match may return socket where sk_bound_dev_if = 0
instead of completely matching one. Isn't it?

Cyrill