linux-kernel - Re: [PATCH v2 1/3] xen-blkback: Advertise feature-persistent as user requested

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]

Date:   Fri, 2 Sep 2022 13:08:37 +0200
From:   Juergen Gross <jgross@...e.com>
To:     Pratyush Yadav <ptyadav@...zon.de>, SeongJae Park <sj@...nel.org>
Cc:     roger.pau@...rix.com, marmarek@...isiblethingslab.com,
        mheyne@...zon.de, 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 v2 1/3] xen-blkback: Advertise feature-persistent as user
 requested

On 02.09.22 11:53, Pratyush Yadav wrote:
> On 31/08/22 04:58PM, SeongJae Park wrote:
>> The advertisement of the persistent grants feature (writing
>> 'feature-persistent' to xenbus) should mean not the decision for using
>> the feature but only the availability of the feature.  However, commit
>> aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent
>> grants") made a field of blkback, which was a place for saving only the
>> negotiation result, to be used for yet another purpose: caching of the
>> 'feature_persistent' parameter value.  As a result, the advertisement,
>> which should follow only the parameter value, becomes inconsistent.
>>
>> This commit fixes the misuse of the semantic by making blkback saves the
>> parameter value in a separate place and advertises the support based on
>> only the saved value.
>>
>> Fixes: aac8a70db24b ("xen-blkback: add a parameter for disabling of persistent grants")
>> Cc: <stable@...r.kernel.org> # 5.10.x
>> Suggested-by: Juergen Gross <jgross@...e.com>
>> Signed-off-by: SeongJae Park <sj@...nel.org>
>> ---
>>   drivers/block/xen-blkback/common.h | 3 +++
>>   drivers/block/xen-blkback/xenbus.c | 6 ++++--
>>   2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
>> index bda5c815e441..a28473470e66 100644
>> --- a/drivers/block/xen-blkback/common.h
>> +++ b/drivers/block/xen-blkback/common.h
>> @@ -226,6 +226,9 @@ struct xen_vbd {
>>          sector_t                size;
>>          unsigned int            flush_support:1;
>>          unsigned int            discard_secure:1;
>> +       /* Connect-time cached feature_persistent parameter value */
>> +       unsigned int            feature_gnt_persistent_parm:1;
> 
> Continuing over from the previous version:
> 
>>> If feature_gnt_persistent_parm is always going to be equal to
>>> feature_persistent, then why introduce it at all? Why not just use
>>> feature_persistent directly? This way you avoid adding an extra flag
>>> whose purpose is not immediately clear, and you also avoid all the
>>> mess with setting this flag at the right time.
>>
>> Mainly because the parameter should read twice (once for
>> advertisement, and once later just before the negotitation, for
>> checking if we advertised or not), and the user might change the
>> parameter value between the two reads.
>>
>> For the detailed available sequence of the race, you could refer to the
>> prior conversation[1].
>>
>> [1] https://lore.kernel.org/linux-block/20200922111259.GJ19254@Air-de-Roger/
> 
> Okay, I see. Thanks for the pointer. But still, I think it would be
> better to not maintain two copies of the value. How about doing:
> 
> 	blkif->vbd.feature_gnt_persistent =
> 		xenbus_read_unsigned(dev->nodename, "feature-persistent", 0) &&
> 		xenbus_read_unsigned(dev->otherend, "feature-persistent", 0);
> 
> This makes it quite clear that we only enable persistent grants if
> _both_ ends support it. We can do the same for blkfront.

I prefer it as is, as it will not rely on nobody having modified the
Xenstore node (which would in theory be possible).


Juergen

Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3099 bytes)

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (496 bytes)