On 6/27/24 09:40, Jakub Sitnicki wrote:
> On Wed, Jun 26, 2024 at 12:19 PM +02, Michal Luczaj wrote:
>> On 6/24/24 16:15, Jakub Sitnicki wrote:
>>> On Sun, Jun 23, 2024 at 12:25 AM +02, Michal Luczaj wrote:
>>>> AF_UNIX socket tracks the most recent OOB packet (in its receive queue)
>>>> with an `oob_skb` pointer. BPF redirecting does not account for that: when
>>>> an OOB packet is moved between sockets, `oob_skb` is left outdated. This
>>>> results in a single skb that may be accessed from two different sockets.
>>>>
>>>> Take the easy way out: silently drop MSG_OOB data targeting any socket that
>>>> is in a sockmap or a sockhash. Note that such silent drop is akin to the
>>>> fate of redirected skb's scm_fp_list (SCM_RIGHTS, SCM_CREDENTIALS).
>>>>
>>>> For symmetry, forbid MSG_OOB in unix_bpf_recvmsg().
>>>>
>>>> Suggested-by: Kuniyuki Iwashima <kuniyu@xxxxxxxxxx>
>>>> Fixes: 314001f0bf92 ("af_unix: Add OOB support")
>>>> Signed-off-by: Michal Luczaj <mhal@xxxxxxx>
>>>> ---
>>>
>>> [+CC Cong who authored ->read_skb]
>>>
>>> I'm guessing you have a test program that you're developing the fix
>>> against. Would you like to extend the test case for sockmap redirect
>>> from unix stream [1] to incorporate it?
>>>
>>> Sadly unix_inet_redir_to_connected needs a fix first because it
>>> hardcodes sotype to SOCK_DGRAM.
>>
>> Ugh, my last two replies got silently dropped by vger. Is there any way to
>> tell what went wrong?
>
> Not sure if it was vger or lore archive. Your reply hit my inbox but is
> nowhere to be found in the archive:> [...]
24h later mailer daemon revealed that my SMTP server got (temporarily) on
the Spamhaus Blocklist. Oh well.
>> So, again, sure, I'll extend the sockmap redirect test.
>
> Appreciate the help with adding a regression test, if time allows.
> Fixes are of course very welcome even without them.
No problem, fix along with the test sent. Let me know what you think.
>> And regarding Rao's comment, I took a look and I think sockmap'ed TCP OOB
>> does indeed act the same way. I'll try to add that into selftest as well.n
>
> Right, it does sound like we're not clearing the offset kept in
> tcp_sock::urg_data when skb is redirected.
Yeah, so I also wanted to extend the TCP's redir_to_connected(), but is
that code correct? It seems to be testing REDIR_INGRESS, yet
prog_stream_verdict() doesn't run bpf_sk_redirect_map() with the
BPF_F_INGRESS flag.
Thanks,
Michal
