Skip to contentSkip to conversation

Fixes for raw diag sockets handling

PATCH In review Cyrill Gorcunov 4 participants 6 messages Nov 2, 2016 – Nov 3, 2016

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover Fixes for raw diag sockets handling

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

#1 lore raw
Quoted 16 lines · Cyrill Gorcunov

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

Quoted 6 lines
	 (!(__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.

#2 lore raw
Quoted 5 lines · David Ahern
Quoted 2 lines

@__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

#3 lore raw
Quoted 7 lines · Cyrill Gorcunov
Quoted 5 lines · David Ahern
Quoted 2 lines

@__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.

Quoted 6 lines

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
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 -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
#4 lore raw
Quoted 8 lines · David Ahern

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.

Quoted 7 lines
root@jessie4:~# ss -ap '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))
Quoted 8 lines
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

maintainer
#5 lore raw
In reply to Cyrill Gorcunov

From: Cyrill Gorcunov <[email protected]>
Date: Wed, 02 Nov 2016 15:36:30 +0300

Quoted 3 lines

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.