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: Mon, 14 Dec 2020 12:06:48 +0800 From: Jason Wang <jasowang@...hat.com> To: Willem de Bruijn <willemdebruijn.kernel@...il.com> Cc: wangyunjian <wangyunjian@...wei.com>, "Michael S. Tsirkin" <mst@...hat.com>, "virtualization@...ts.linux-foundation.org" <virtualization@...ts.linux-foundation.org>, Network Development <netdev@...r.kernel.org>, "Lilijun (Jerry)" <jerry.lilijun@...wei.com>, chenchanghu <chenchanghu@...wei.com>, xudingke <xudingke@...wei.com>, "huangbin (J)" <brian.huangbin@...wei.com>, Willem de Bruijn <willemb@...gle.com> Subject: Re: [PATCH net v2] tun: fix ubuf refcount incorrectly on error path On 2020/12/14 上午11:56, Willem de Bruijn wrote: > On Sun, Dec 13, 2020 at 10:54 PM Willem de Bruijn > <willemdebruijn.kernel@...il.com> wrote: >> On Sun, Dec 13, 2020 at 10:30 PM Jason Wang <jasowang@...hat.com> wrote: >>> >>> On 2020/12/14 上午9:32, Willem de Bruijn wrote: >>>> On Sat, Dec 12, 2020 at 7:18 PM Willem de Bruijn >>>> <willemdebruijn.kernel@...il.com> wrote: >>>>>>>> afterwards, the error handling in vhost handle_tx() will try to >>>>>>>> decrease the same refcount again. This is wrong and fix this by delay >>>>>>>> copying ubuf_info until we're sure there's no errors. >>>>>>> I think the right approach is to address this in the error paths, rather than >>>>>>> complicate the normal datapath. >>>>>>> >>>>>>> Is it sufficient to suppress the call to vhost_net_ubuf_put in the handle_tx >>>>>>> sendmsg error path, given that vhost_zerocopy_callback will be called on >>>>>>> kfree_skb? >>>>>> We can not call kfree_skb() until the skb was created. >>>>>> >>>>>>> Or alternatively clear the destructor in drop: >>>>>> The uarg->callback() is called immediately after we decide do datacopy >>>>>> even if caller want to do zerocopy. If another error occurs later, the vhost >>>>>> handle_tx() will try to decrease it again. >>>>> Oh right, I missed the else branch in this path: >>>>> >>>>> /* copy skb_ubuf_info for callback when skb has no error */ >>>>> if (zerocopy) { >>>>> skb_shinfo(skb)->destructor_arg = msg_control; >>>>> skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY; >>>>> skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG; >>>>> } else if (msg_control) { >>>>> struct ubuf_info *uarg = msg_control; >>>>> uarg->callback(uarg, false); >>>>> } >>>>> >>>>> So if handle_tx_zerocopy calls tun_sendmsg with ubuf_info (and thus a >>>>> reference to release), there are these five options: >>>>> >>>>> 1. tun_sendmsg succeeds, ubuf_info is associated with skb. >>>>> reference released from kfree_skb calling vhost_zerocopy_callback later >>>>> >>>>> 2. tun_sendmsg succeeds, ubuf_info is released immediately, as skb is >>>>> not zerocopy. >>>>> >>>>> 3. tun_sendmsg fails before creating skb, handle_tx_zerocopy correctly >>>>> cleans up on receiving error from tun_sendmsg. >>>>> >>>>> 4. tun_sendmsg fails after creating skb, but with copying: decremented >>>>> at branch shown above + again in handle_tx_zerocopy >>>>> >>>>> 5. tun_sendmsg fails after creating skb, with zerocopy: decremented at >>>>> kfree_skb in drop: + again in handle_tx_zerocopy >>>>> >>>>> Since handle_tx_zerocopy has no idea whether on error 3, 4 or 5 >>>>> occurred, >>>> Actually, it does. If sendmsg returns an error, it can test whether >>>> vq->heads[nvq->upend_idx].len != VHOST_DMA_IN_PROGRESS. >>> >>> Just to make sure I understand this. Any reason for it can't be >>> VHOST_DMA_IN_PROGRESS here? >> It can be, and it will be if tun_sendmsg returns EINVAL before >> assigning the skb destructor. > I meant returns an error, not necessarily only EINVAL. > >> Only if tun_sendmsg released the zerocopy state through >> kfree_skb->vhost_zerocopy_callback will it have been updated to >> VHOST_DMA_DONE_LEN. And only then must the caller not try to release >> the state again. > I see. So I tend to fix this in vhost instead of tun to be consistent with the current error handling in handle_tx_zerocopy(). Thanks
Powered by blists - more mailing lists