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
| ||
|
Date: Sun, 25 Feb 2018 11:20:27 -0500 From: Sowmini Varadhan <sowmini.varadhan@...cle.com> To: Willem de Bruijn <willemdebruijn.kernel@...il.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 (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() > 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? > > > + > > + 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? > > + 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? > > + ncookies = rds_recvmsg_zcookie(rs, msg); Will take care of the remaining comments in V3.
Powered by blists - more mailing lists