From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3D4FCC433FE for ; Wed, 19 Jan 2022 20:14:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S244447AbiASUOM (ORCPT ); Wed, 19 Jan 2022 15:14:12 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40038 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230196AbiASUOK (ORCPT ); Wed, 19 Jan 2022 15:14:10 -0500 Received: from mail-lf1-x133.google.com (mail-lf1-x133.google.com [IPv6:2a00:1450:4864:20::133]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6E7F4C061574; Wed, 19 Jan 2022 12:14:10 -0800 (PST) Received: by mail-lf1-x133.google.com with SMTP id o15so12702822lfo.11; Wed, 19 Jan 2022 12:14:10 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=kdECfyTlH9oIX1INZDhG7zfW8t8vpJV5SIYtDDGY1nc=; b=mVfChiTRlR52otgIciCZ+fVVRi8nHooY5ndpTYsWLVFk3NlEGYw4jeLbQHYh6BKxxI vlxeIjppYp65Y01iqLnfjYe1HSgQi9LnuZAcYROl76L0wkn/ihtCvLQJP11hZ549duCd U+yMDLD6IJtQFaiVbg17mGhD/Bg+xszj8xyEQOC91h+MO2S0Xi1Hf6aZUYZiaDClLJG8 Fvply5AVXxI8ACmV5Z0a/SI4Lchm7P5apnX3JwAZnkBQlm3KZqi/lUwsPmY08HV/TrxU vgpHEQ1HnG3gBl4A4+7Jl1y1yC8WcN9BP9/VRG9PAosELLNmUq2p/ghLI9SuSZPAdCtb rbGQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=kdECfyTlH9oIX1INZDhG7zfW8t8vpJV5SIYtDDGY1nc=; b=hvaAmr0WcyUXSQFLzd5qA6T0ZiIhAc7pDxl6Zz1jLedmWB1jy4XNBM9Gx9Mr+B7UuW 48dglGJoG+Szir1YdPrmU8IQzKhEUWDALOFtVOt5iJBjoxl401PueY985u6LURemVKET duSWy3I0FRzhBpWx4t8l1Y+0+Jb4SFwStdlWCukwtfrZwFHPQF6S6QpQkYsq1MTA+4Qv BEbTZmhO1uy51O2mmfaiikAGujAVtlTQve3cryIvvcQQZn/kfvo+u1heoQBzxIA/Ahgm allbXXrUzKJXinffKc/PxFrbPytwmV9hE/adI1qhFI+cGAGGQkulqUaFMjP3/Mda4JNY ADVQ== X-Gm-Message-State: AOAM531cN0a/hE8+Ru7bGXnZ8gkJKlCBJV0cXdjDaAzFeEMmuJpuWLkl PE/wB33SPkF0QPUmnGNrhe6LmjpP+VTJAzoqDLg= X-Google-Smtp-Source: ABdhPJy7lyRzLjKdMwTgSEYWLm+wfhMR600om5OGmmvwN2/K/WUWdz4wmJe8VFPXMmuG2eJ+qll39mfrBiWSWVSxGIQ= X-Received: by 2002:a05:6512:39d5:: with SMTP id k21mr25180456lfu.474.1642623248573; Wed, 19 Jan 2022 12:14:08 -0800 (PST) MIME-Version: 1.0 References: <20220109095516.3250392-1-yanminglr@gmail.com> <20220110004419.GA435914@anparri> <20220114191307.uu2oel7wbxhiqe56@liuwe-devbox-debian-v2> In-Reply-To: From: Yanming Liu Date: Thu, 20 Jan 2022 04:13:45 +0800 Message-ID: Subject: Re: [PATCH v2] hv: account for packet descriptor in maximum packet size To: "Michael Kelley (LINUX)" Cc: Wei Liu , Andrea Parri , "linux-hyperv@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , "linux-fbdev@vger.kernel.org" , KY Srinivasan , Haiyang Zhang , Stephen Hemminger , Dexuan Cui , "drawat.floss@gmail.com" , "airlied@linux.ie" , "daniel@ffwll.ch" , "lkmlabelt@gmail.com" Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 20, 2022 at 2:12 AM Michael Kelley (LINUX) wrote: > > From: Wei Liu Sent: Friday, January 14, 2022 11:13 AM > > > > On Mon, Jan 10, 2022 at 01:44:19AM +0100, Andrea Parri wrote: > > > (Extending Cc: list,) > > > > > > On Sun, Jan 09, 2022 at 05:55:16PM +0800, Yanming Liu wrote: > > > > Commit adae1e931acd ("Drivers: hv: vmbus: Copy packets sent by Hyper-V > > > > out of the ring buffer") introduced a notion of maximum packet size in > > > > vmbus channel and used that size to initialize a buffer holding all > > > > incoming packet along with their vmbus packet header. Currently, some > > > > vmbus drivers set max_pkt_size to the size of their receive buffer > > > > passed to vmbus_recvpacket, however vmbus_open expects this size to also > > > > include vmbus packet header. This leads to corruption of the ring buffer > > > > state when receiving a maximum sized packet. > > > > > > > > Specifically, in hv_balloon I have observed of a dm_unballoon_request > > > > message of 4096 bytes being truncated to 4080 bytes. When the driver > > > > tries to read next packet it starts from a wrong read_index, receives > > > > garbage and prints a lot of "Unhandled message: type: " in > > > > dmesg. > > > > > > > > The same mismatch also happens in hv_fcopy, hv_kvp, hv_snapshot, > > > > hv_util, hyperv_drm and hyperv_fb, though bad cases are not observed > > > > yet. > > > > > > > > Allocate the buffer with HV_HYP_PAGE_SIZE more bytes to make room for > > > > the descriptor, assuming the vmbus packet header will never be larger > > > > than HV_HYP_PAGE_SIZE. This is essentially free compared to just adding > > > > 'sizeof(struct vmpacket_descriptor)' because these buffers are all more > > > > than HV_HYP_PAGE_SIZE bytes so kmalloc rounds them up anyway. > > > > > > > > Fixes: adae1e931acd ("Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the ring buffer") > > > > Suggested-by: Andrea Parri (Microsoft) > > > > Signed-off-by: Yanming Liu > > > > > > Thanks for sorting this out; the patch looks good to me: > > > > > > Reviewed-by: Andrea Parri (Microsoft) > > > > > > > Thanks. I will pick this up after 5.17-rc1 is out. > > > > Wei. > > I'm NACK'ing this set of changes. I've spent some further time investigating, > so let me explain. > > I'm good with the overall approach of fixing individual drivers to set the > max_pkt_size to account for the VMbus packet header, as this is an > important aspect that was missed in the original coding. But interestingly, > all but one of the miscellaneous VMbus drivers allocate significantly more > receive buffer space than is actually needed, and the max_pkt_size matching > that receive buffer size is already bigger than needed. In all these > cases, there is already plenty of space for the VMbus packet header. > Appreciate for the additional insight on what Hyper-V would do! > These hv-util.c drivers allocate a receive buffer 4 Kbytes in size, and all > receive only small fixed-size packets: heartbeat, shutdown, timesync. > I don't think any changes are needed for these drivers because the default > max_pkt_size value of 4 Kbytes bytes is plenty of space even when > accounting for the VMbus packet header. > > The VSS driver in hv_snapshot.c allocates a receive buffer of 8 Kbytes > and sets max_pkt_size to 8 Kbytes. But the received messages are > all fixed size and small. I don't know why the driver uses an 8 Kbyte > receive buffer instead of 4 Kbytes, but the current settings are > more than sufficient. > Well, I'm not sure, on August 2021 there was a patch changing max_pkt_size to 8 KiB for VSS driver: https://lore.kernel.org/linux-hyperv/20210825190217.qh2c6yq5qr3ntum5@liuwe-devbox-debian-v2/T/ The patch mentioned a 6304 bytes VSS message. Which is part of the reason I tried to address the more "general" problem of potentially mismatching buffer size. > The FCOPY driver in hv_fcopy.c allocates a receive buffer of 8 Kbytes > and sets max_pkt_size to 8 Kbytes. The received messages have > some header overhead plus up to 6 Kbytes of data, so the 8 Kbyte > receive buffer is definitely needed. And while this one is a little > closer to filling up the available receive space than the previous > ones, there's still plenty of room for the VMbus packet header. I > don't think any changes are needed. > > The KVP driver in hv_kvp.c allocates a receive buffer of 16 Kbytes > and sets max_pkt_size to 16 Kbytes. From what I can tell, the > received messages max out at close to 4 Kbytes. Key exchange > messages have 512 bytes of key name and 2048 bytes of key > value, plus some header overhead. ipaddr_value messages > are the largest, with 3 IP addresses @ 1024 bytes each, plus > a gateway with 512 bytes, and an adapter ID with 128 bytes. > But altogether, that is still less than 4096. I don't know why > the receive buffer is 16 Kbytes, but it is plenty big and no > changes are needed. > > The two frame buffer drivers also use 16 Kbyte receive buffers > and set max_pkt_size to 16 Kbytes. Again, this looks to be overkill > as the messages received are mostly fixed size. One message > returns a variable size list of supported screen resolutions, but > each entry in the list is only 4 bytes, and we're looking at a few > tens of resolutions at the very most. Again, no changes are > needed. > > After all this analysis, the balloon driver is the only one that > needs changing. It uses a 4 Kbyte receive buffer, and indeed > Hyper-V may fill that receive buffer in the case of unballoon > messages. And that where the original problem was observed. > > Two other aspects for completeness. First, all these drivers > do protocol version negotiation with the Hyper-V host. The > negotiation includes a variable-size list of supported versions. > Each version in the list takes 4 bytes, but there would never > be enough different versions to come close to filling a 4 Kbyte > buffer. So there's no problem here. > > The other lurking issue is the 'offset8' field in the VMbus > packet header, which says where the payload starts relative > to the VMbus packet header. In practice, this value is always > small, so there's no significant additional space to account > for. While it's theoretically possible that Hyper-V could use > a much larger value, and cause max_pkt_size to be exceeded, > there's no real way to fix this problem. Adding an extra page > to max_pkt_size, as it done in this patch, certainly provides > some extra room, but doesn't guarantee the problem can't > happen. But since there's no indication Hyper-V would ever > put a big value into offset8, I don't think we need to worry > about the possibility. > Thanks for confirming this! > My bottom-line: Let's fix the balloon driver. But now > that we know the other drivers are safe "as is", let's leave > them unchanged and not waste the additional memory. > Sure, after all I just want a working kernel :) > Michael From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id E202CC433F5 for ; Thu, 20 Jan 2022 08:47:48 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 3709E10E922; Thu, 20 Jan 2022 08:47:43 +0000 (UTC) Received: from mail-lf1-x129.google.com (mail-lf1-x129.google.com [IPv6:2a00:1450:4864:20::129]) by gabe.freedesktop.org (Postfix) with ESMTPS id 96BF410EC0A for ; Wed, 19 Jan 2022 20:14:10 +0000 (UTC) Received: by mail-lf1-x129.google.com with SMTP id y15so4506045lfa.9 for ; Wed, 19 Jan 2022 12:14:10 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=kdECfyTlH9oIX1INZDhG7zfW8t8vpJV5SIYtDDGY1nc=; b=mVfChiTRlR52otgIciCZ+fVVRi8nHooY5ndpTYsWLVFk3NlEGYw4jeLbQHYh6BKxxI vlxeIjppYp65Y01iqLnfjYe1HSgQi9LnuZAcYROl76L0wkn/ihtCvLQJP11hZ549duCd U+yMDLD6IJtQFaiVbg17mGhD/Bg+xszj8xyEQOC91h+MO2S0Xi1Hf6aZUYZiaDClLJG8 Fvply5AVXxI8ACmV5Z0a/SI4Lchm7P5apnX3JwAZnkBQlm3KZqi/lUwsPmY08HV/TrxU vgpHEQ1HnG3gBl4A4+7Jl1y1yC8WcN9BP9/VRG9PAosELLNmUq2p/ghLI9SuSZPAdCtb rbGQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=kdECfyTlH9oIX1INZDhG7zfW8t8vpJV5SIYtDDGY1nc=; b=IymmJV+fwpQrCEnlTsDLUtgBbqqdjH5+GmYg4SKHLElP7Uf0PXkLVIescRm86Pg9AV OOX015BUh0HsLgCAoEmepCu4X3LyCvJDdKE+DrJGIgzzd5bSYbyWsgHq9Y44TwJr4+0P bhQmlG7ZqRVPZUnQ9GAULlXTeH4drz7zCt4z/6badJpVXwXSI2kET2qyXWlv1LMU3eLF JsHeG7RhRFrdRnjX/HKXM6ovZ5FCO3Qakrg/LXZN30q1pAHL+UCMfNe2+1LapOGFv/3R gDaftJcsSl9vMBEPtEy/pJb0Di1+3D2dzZGYZL4uwNZaCZ+F3ZgqmvsQNJhf1slVKUVh n0fQ== X-Gm-Message-State: AOAM531nH6ozCJwZ5vp28Ow60o/pXx+h7yn5Rwyxt85JrgmAFvm/Y5ko 2YlwT3rvD2C4Ymtk0A0HyN4z6YT+W3lXr6VyyDQ= X-Google-Smtp-Source: ABdhPJy7lyRzLjKdMwTgSEYWLm+wfhMR600om5OGmmvwN2/K/WUWdz4wmJe8VFPXMmuG2eJ+qll39mfrBiWSWVSxGIQ= X-Received: by 2002:a05:6512:39d5:: with SMTP id k21mr25180456lfu.474.1642623248573; Wed, 19 Jan 2022 12:14:08 -0800 (PST) MIME-Version: 1.0 References: <20220109095516.3250392-1-yanminglr@gmail.com> <20220110004419.GA435914@anparri> <20220114191307.uu2oel7wbxhiqe56@liuwe-devbox-debian-v2> In-Reply-To: From: Yanming Liu Date: Thu, 20 Jan 2022 04:13:45 +0800 Message-ID: Subject: Re: [PATCH v2] hv: account for packet descriptor in maximum packet size To: "Michael Kelley (LINUX)" Content-Type: text/plain; charset="UTF-8" X-Mailman-Approved-At: Thu, 20 Jan 2022 08:47:25 +0000 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Andrea Parri , Wei Liu , Stephen Hemminger , "lkmlabelt@gmail.com" , "linux-hyperv@vger.kernel.org" , "airlied@linux.ie" , Haiyang Zhang , Dexuan Cui , "linux-fbdev@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "drawat.floss@gmail.com" , KY Srinivasan Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Thu, Jan 20, 2022 at 2:12 AM Michael Kelley (LINUX) wrote: > > From: Wei Liu Sent: Friday, January 14, 2022 11:13 AM > > > > On Mon, Jan 10, 2022 at 01:44:19AM +0100, Andrea Parri wrote: > > > (Extending Cc: list,) > > > > > > On Sun, Jan 09, 2022 at 05:55:16PM +0800, Yanming Liu wrote: > > > > Commit adae1e931acd ("Drivers: hv: vmbus: Copy packets sent by Hyper-V > > > > out of the ring buffer") introduced a notion of maximum packet size in > > > > vmbus channel and used that size to initialize a buffer holding all > > > > incoming packet along with their vmbus packet header. Currently, some > > > > vmbus drivers set max_pkt_size to the size of their receive buffer > > > > passed to vmbus_recvpacket, however vmbus_open expects this size to also > > > > include vmbus packet header. This leads to corruption of the ring buffer > > > > state when receiving a maximum sized packet. > > > > > > > > Specifically, in hv_balloon I have observed of a dm_unballoon_request > > > > message of 4096 bytes being truncated to 4080 bytes. When the driver > > > > tries to read next packet it starts from a wrong read_index, receives > > > > garbage and prints a lot of "Unhandled message: type: " in > > > > dmesg. > > > > > > > > The same mismatch also happens in hv_fcopy, hv_kvp, hv_snapshot, > > > > hv_util, hyperv_drm and hyperv_fb, though bad cases are not observed > > > > yet. > > > > > > > > Allocate the buffer with HV_HYP_PAGE_SIZE more bytes to make room for > > > > the descriptor, assuming the vmbus packet header will never be larger > > > > than HV_HYP_PAGE_SIZE. This is essentially free compared to just adding > > > > 'sizeof(struct vmpacket_descriptor)' because these buffers are all more > > > > than HV_HYP_PAGE_SIZE bytes so kmalloc rounds them up anyway. > > > > > > > > Fixes: adae1e931acd ("Drivers: hv: vmbus: Copy packets sent by Hyper-V out of the ring buffer") > > > > Suggested-by: Andrea Parri (Microsoft) > > > > Signed-off-by: Yanming Liu > > > > > > Thanks for sorting this out; the patch looks good to me: > > > > > > Reviewed-by: Andrea Parri (Microsoft) > > > > > > > Thanks. I will pick this up after 5.17-rc1 is out. > > > > Wei. > > I'm NACK'ing this set of changes. I've spent some further time investigating, > so let me explain. > > I'm good with the overall approach of fixing individual drivers to set the > max_pkt_size to account for the VMbus packet header, as this is an > important aspect that was missed in the original coding. But interestingly, > all but one of the miscellaneous VMbus drivers allocate significantly more > receive buffer space than is actually needed, and the max_pkt_size matching > that receive buffer size is already bigger than needed. In all these > cases, there is already plenty of space for the VMbus packet header. > Appreciate for the additional insight on what Hyper-V would do! > These hv-util.c drivers allocate a receive buffer 4 Kbytes in size, and all > receive only small fixed-size packets: heartbeat, shutdown, timesync. > I don't think any changes are needed for these drivers because the default > max_pkt_size value of 4 Kbytes bytes is plenty of space even when > accounting for the VMbus packet header. > > The VSS driver in hv_snapshot.c allocates a receive buffer of 8 Kbytes > and sets max_pkt_size to 8 Kbytes. But the received messages are > all fixed size and small. I don't know why the driver uses an 8 Kbyte > receive buffer instead of 4 Kbytes, but the current settings are > more than sufficient. > Well, I'm not sure, on August 2021 there was a patch changing max_pkt_size to 8 KiB for VSS driver: https://lore.kernel.org/linux-hyperv/20210825190217.qh2c6yq5qr3ntum5@liuwe-devbox-debian-v2/T/ The patch mentioned a 6304 bytes VSS message. Which is part of the reason I tried to address the more "general" problem of potentially mismatching buffer size. > The FCOPY driver in hv_fcopy.c allocates a receive buffer of 8 Kbytes > and sets max_pkt_size to 8 Kbytes. The received messages have > some header overhead plus up to 6 Kbytes of data, so the 8 Kbyte > receive buffer is definitely needed. And while this one is a little > closer to filling up the available receive space than the previous > ones, there's still plenty of room for the VMbus packet header. I > don't think any changes are needed. > > The KVP driver in hv_kvp.c allocates a receive buffer of 16 Kbytes > and sets max_pkt_size to 16 Kbytes. From what I can tell, the > received messages max out at close to 4 Kbytes. Key exchange > messages have 512 bytes of key name and 2048 bytes of key > value, plus some header overhead. ipaddr_value messages > are the largest, with 3 IP addresses @ 1024 bytes each, plus > a gateway with 512 bytes, and an adapter ID with 128 bytes. > But altogether, that is still less than 4096. I don't know why > the receive buffer is 16 Kbytes, but it is plenty big and no > changes are needed. > > The two frame buffer drivers also use 16 Kbyte receive buffers > and set max_pkt_size to 16 Kbytes. Again, this looks to be overkill > as the messages received are mostly fixed size. One message > returns a variable size list of supported screen resolutions, but > each entry in the list is only 4 bytes, and we're looking at a few > tens of resolutions at the very most. Again, no changes are > needed. > > After all this analysis, the balloon driver is the only one that > needs changing. It uses a 4 Kbyte receive buffer, and indeed > Hyper-V may fill that receive buffer in the case of unballoon > messages. And that where the original problem was observed. > > Two other aspects for completeness. First, all these drivers > do protocol version negotiation with the Hyper-V host. The > negotiation includes a variable-size list of supported versions. > Each version in the list takes 4 bytes, but there would never > be enough different versions to come close to filling a 4 Kbyte > buffer. So there's no problem here. > > The other lurking issue is the 'offset8' field in the VMbus > packet header, which says where the payload starts relative > to the VMbus packet header. In practice, this value is always > small, so there's no significant additional space to account > for. While it's theoretically possible that Hyper-V could use > a much larger value, and cause max_pkt_size to be exceeded, > there's no real way to fix this problem. Adding an extra page > to max_pkt_size, as it done in this patch, certainly provides > some extra room, but doesn't guarantee the problem can't > happen. But since there's no indication Hyper-V would ever > put a big value into offset8, I don't think we need to worry > about the possibility. > Thanks for confirming this! > My bottom-line: Let's fix the balloon driver. But now > that we know the other drivers are safe "as is", let's leave > them unchanged and not waste the additional memory. > Sure, after all I just want a working kernel :) > Michael