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] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220826142658.GA77627@dev-dsk-mheyne-1b-c1362c4d.eu-west-1.amazon.com>
Date:   Fri, 26 Aug 2022 14:26:58 +0000
From:   Maximilian Heyne <mheyne@...zon.de>
To:     SeongJae Park <sj@...nel.org>
CC:     <jgross@...e.com>, <roger.pau@...rix.com>,
        <marmarek@...isiblethingslab.com>,
        <xen-devel@...ts.xenproject.org>, <axboe@...nel.dk>,
        <linux-block@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <stable@...r.kernel.org>
Subject: Re: [PATCH 2/2] xen-blkfront: Advertise feature-persistent as user requested

On Thu, Aug 25, 2022 at 04:15:11PM +0000, SeongJae Park wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
> when connect") made blkback to advertise its support of the persistent
> grants feature only if the user sets the 'feature_persistent' parameter
> of the driver and the frontend advertised its support of the feature.
> However, following commit 402c43ea6b34 ("xen-blkfront: Apply
> 'feature_persistent' parameter when connect") made the blkfront to work
> in the same way.  That is, blkfront also advertises its support of the
> persistent grants feature only if the user sets the 'feature_persistent'
> parameter of the driver and the backend advertised its support of the
> feature.
> 
> Hence blkback and blkfront will never advertise their support of the
> feature but wait until the other advertises the support, even though
> users set the 'feature_persistent' parameters of the drivers.  As a
> result, the persistent grants feature is disabled always regardless of
> the 'feature_persistent' values[1].
> 
> The problem comes from the misuse of the semantic of the advertisement
> of the feature.  The advertisement of the feature should means only
> availability of the feature not the decision for using the feature.
> However, current behavior is working in the wrong way.
> 
> This commit fixes the issue by making the blkfront advertises its
> support of the feature as user requested via 'feature_persistent'
> parameter regardless of the otherend's support of the feature.
> 
> [1] https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f7cd@suse.com/
> 
> Fixes: 402c43ea6b34 ("xen-blkfront: Apply 'feature_persistent' parameter when connect")
> Cc: <stable@...r.kernel.org> # 5.10.x
> Reported-by: Marek Marczykowski-Górecki <marmarek@...isiblethingslab.com>
> Suggested-by: Juergen Gross <jgross@...e.com>
> Signed-off-by: SeongJae Park <sj@...nel.org>
> ---
>  drivers/block/xen-blkfront.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 8e56e69fb4c4..dfae08115450 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -213,6 +213,9 @@ struct blkfront_info
>         unsigned int feature_fua:1;
>         unsigned int feature_discard:1;
>         unsigned int feature_secdiscard:1;
> +       /* Connect-time cached feature_persistent parameter */
> +       unsigned int feature_persistent_parm:1;
> +       /* Persistent grants feature negotiation result */
>         unsigned int feature_persistent:1;
>         unsigned int bounce:1;
>         unsigned int discard_granularity;
> @@ -1848,7 +1851,7 @@ static int talk_to_blkback(struct xenbus_device *dev,
>                 goto abort_transaction;
>         }
>         err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
> -                       info->feature_persistent);
> +                       info->feature_persistent_parm);
>         if (err)
>                 dev_warn(&dev->dev,
>                          "writing persistent grants feature to xenbus");
> @@ -2281,7 +2284,8 @@ static void blkfront_gather_backend_features(struct blkfront_info *info)
>         if (xenbus_read_unsigned(info->xbdev->otherend, "feature-discard", 0))
>                 blkfront_setup_discard(info);
> 
> -       if (feature_persistent)
> +       info->feature_persistent_parm = feature_persistent;

I think setting this here is too late because "feature-persistent" was already
written to xenstore via talk_to_blkback but with default 0. So during the
connect blkback will not see that the guest supports the feature and falls back
to no persistent grants.

Tested only this patch with some hacky dom0 kernel that doesn't have the patch
from your series yet. Will do more testing next week.

> +       if (info->feature_persistent_parm)
>                 info->feature_persistent =
>                         !!xenbus_read_unsigned(info->xbdev->otherend,
>                                                "feature-persistent", 0);
> --
> 2.25.1
> 



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ