* [patch net-next 0/2] Fixes for raw diag sockets handling
@ 2016-11-02 12:36 Cyrill Gorcunov
2016-11-02 15:10 ` David Ahern
2016-11-03 19:26 ` David Miller
0 siblings, 2 replies; 6+ messages in thread
From: Cyrill Gorcunov @ 2016-11-02 12:36 UTC (permalink / raw)
To: netdev
Cc: Eric Dumazet, David S. Miller, David Ahern, Andrey Vagin,
Stephen Hemminger
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch net-next 0/2] Fixes for raw diag sockets handling
2016-11-02 12:36 [patch net-next 0/2] Fixes for raw diag sockets handling Cyrill Gorcunov
@ 2016-11-02 15:10 ` David Ahern
2016-11-02 15:29 ` Cyrill Gorcunov
2016-11-03 19:26 ` David Miller
1 sibling, 1 reply; 6+ messages in thread
From: David Ahern @ 2016-11-02 15:10 UTC (permalink / raw)
To: Cyrill Gorcunov, netdev
Cc: Eric Dumazet, David S. Miller, Andrey Vagin, Stephen Hemminger
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch net-next 0/2] Fixes for raw diag sockets handling
2016-11-02 15:10 ` David Ahern
@ 2016-11-02 15:29 ` Cyrill Gorcunov
2016-11-02 15:36 ` David Ahern
0 siblings, 1 reply; 6+ messages in thread
From: Cyrill Gorcunov @ 2016-11-02 15:29 UTC (permalink / raw)
To: David Ahern
Cc: netdev, Eric Dumazet, David S. Miller, Andrey Vagin,
Stephen Hemminger
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch net-next 0/2] Fixes for raw diag sockets handling
2016-11-02 15:29 ` Cyrill Gorcunov
@ 2016-11-02 15:36 ` David Ahern
2016-11-02 15:45 ` Cyrill Gorcunov
0 siblings, 1 reply; 6+ messages in thread
From: David Ahern @ 2016-11-02 15:36 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: netdev, Eric Dumazet, David S. Miller, Andrey Vagin,
Stephen Hemminger
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
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch net-next 0/2] Fixes for raw diag sockets handling
2016-11-02 15:36 ` David Ahern
@ 2016-11-02 15:45 ` Cyrill Gorcunov
0 siblings, 0 replies; 6+ messages in thread
From: Cyrill Gorcunov @ 2016-11-02 15:45 UTC (permalink / raw)
To: David Ahern
Cc: netdev, Eric Dumazet, David S. Miller, Andrey Vagin,
Stephen Hemminger
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
> 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
Cyrill
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch net-next 0/2] Fixes for raw diag sockets handling
2016-11-02 12:36 [patch net-next 0/2] Fixes for raw diag sockets handling Cyrill Gorcunov
2016-11-02 15:10 ` David Ahern
@ 2016-11-03 19:26 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2016-11-03 19:26 UTC (permalink / raw)
To: gorcunov; +Cc: netdev, eric.dumazet, dsa, avagin, stephen
From: Cyrill Gorcunov <gorcunov@openvz.org>
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-11-03 19:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-02 12:36 [patch net-next 0/2] Fixes for raw diag sockets handling Cyrill Gorcunov
2016-11-02 15:10 ` David Ahern
2016-11-02 15:29 ` Cyrill Gorcunov
2016-11-02 15:36 ` David Ahern
2016-11-02 15:45 ` Cyrill Gorcunov
2016-11-03 19:26 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).