netdev - Re: [PATCH V2 net-next 2/3] rds: deliver zerocopy completion notification with data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAF=yD-JKMhCfQVF19uER1XEiA6=BXYSH11G0GMzOj+u83Tbiqg@mail.gmail.com>
Date:   Sun, 25 Feb 2018 12:36:03 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Sowmini Varadhan <sowmini.varadhan@...cle.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Santosh Shilimkar <santosh.shilimkar@...cle.com>
Subject: Re: [PATCH V2 net-next 2/3] rds: deliver zerocopy completion
 notification with data

On Sun, Feb 25, 2018 at 11:20 AM, Sowmini Varadhan
<sowmini.varadhan@...cle.com> wrote:
> On (02/25/18 10:56), Willem de Bruijn wrote:
>> > @@ -91,22 +85,19 @@ static void rds_rm_zerocopy_callback(struct rds_sock *rs,
>> >                 spin_unlock_irqrestore(&q->lock, flags);
>> >                 mm_unaccount_pinned_pages(&znotif->z_mmp);
>> >                 consume_skb(rds_skb_from_znotifier(znotif));
>> > -               sk->sk_error_report(sk);
>> > +               /* caller should wake up POLLIN */
>>
>> sk->sk_data_ready(sk);
>
> yes, this was my first thought, but everything else in rds
> is calling rds_wake_sk_sleep (this is even done in
> rds_recv_incoming(), which actually queues up the data),
> so I chose to align with that model (and call this in the caller
> of rds_rm_zerocopy_callback()

Ah, understood. Perhaps say "wakes" instead of "should wake".
I mistakenly read this as a todo.

>> Without the error queue, the struct no longer needs to be an skb,
>> per se. Converting to a different struct with list_head is definitely
>> a longer patch. But kmalloc will be cheaper than alloc_skb.
>> Perhaps something to try (as separate follow-on work).
>
> right, I was thinking along these exact lines as well,
> and was already planning a follow-up.
>
>> > +       if (!sock_flag(rds_rs_to_sk(rs), SOCK_ZEROCOPY) || !skb_peek(q))
>> > +               return 0;
>>
>> Racy read?
>
> Can you elaborate? I only put the skb_peek to quickly
> bail for sockets that are not using zerocopy at all-
> if you race against something that's queuing data, and
> miss it on the peek, the next read/recv should find it.
> Am I missing some race?

It''s a lockless access. But intentionally so, then. You're right, as long as
the subsequent skb_dequeue handles the case where the queue is
empty, it seems okay to optimistically probe lockless first.

>>
>> > +
>> > +       if (!msg->msg_control ||
>>
>> I'd move this first, so that the cookie queue need not even be probed
>> in the common case.
>
> you mean before the check for SOCK_ZEROCOPY?

Yes

>> > +           msg->msg_controllen < CMSG_SPACE(sizeof(*done)))
>> > +               return 0;
>>
>> if caller does not satisfy the contract on controllen size, can be
>> more explicit and return an error.
>
> if SOCK_ZEROCOPY has been set, but the recv did not specify a cmsghdr,
> you mean?

I mean if SOCK_ZEROCOPY has been set and the caller calls recvmsg
with a control buffer, but one that is too small to handle zerocopy cookie
notifications.

>> > +               ncookies = rds_recvmsg_zcookie(rs, msg);
>
> Will take care of the remaining comments in V3.