linux-kernel - Re: [PATCH] xen/blkfront: correct purging of persistent grants

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, 28 Sep 2018 15:52:56 +0200
From:   Juergen Gross <jgross@...e.com>
To:     Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        linux-kernel@...r.kernel.org, xen-devel@...ts.xenproject.org,
        linux-block@...r.kernel.org
Cc:     roger.pau@...rix.com, konrad.wilk@...cle.com, axboe@...nel.dk
Subject: Re: [PATCH] xen/blkfront: correct purging of persistent grants

On 28/09/2018 15:33, Boris Ostrovsky wrote:
> On 9/28/18 9:13 AM, Juergen Gross wrote:
>> On 28/09/2018 14:45, Boris Ostrovsky wrote:
>>> On 9/28/18 3:28 AM, Juergen Gross wrote:
>>>> Commit a46b53672b2c2e3770b38a4abf90d16364d2584b ("xen/blkfront: cleanup
>>>> stale persistent grants") introduced a regression as purged persistent
>>>> grants were not pu into the list of free grants again. Correct that.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@...e.com>
>>>> ---
>>>>  drivers/block/xen-blkfront.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>>>> index a71d817e900d..429d20131c7e 100644
>>>> --- a/drivers/block/xen-blkfront.c
>>>> +++ b/drivers/block/xen-blkfront.c
>>>> @@ -2670,8 +2670,8 @@ static void purge_persistent_grants(struct blkfront_info *info)
>>>>  			list_del(&gnt_list_entry->node);
>>>>  			gnttab_end_foreign_access(gnt_list_entry->gref, 0, 0UL);
>>>>  			rinfo->persistent_gnts_c--;
>>>> -			__free_page(gnt_list_entry->page);
>>>> -			kfree(gnt_list_entry);
>>>> +			gnt_list_entry->gref = GRANT_INVALID_REF;
>>>> +			list_add_tail(&gnt_list_entry->node, &rinfo->grants);
>>> Sorry, I don't follow this. What is the purpose of removing the grant
>>> from rinfo->grants list with list_del() and then adding it back with
>>> list_add_tail()?
>> The persistent grants are at the list head and the non-persistent ones
>> at the tail.
> 
> Oh, I didn't realize that. But isn't that an optimization (and so not
> following this rule should not cause errors)?

In theory: yes.

The persistent grant handling is rather complicated so I'd like to make
sure not to deviate from the common standards.

When I find some time I want to modify the persistent grant handling to
be more explicit and controlled completely by the frontend (within the
backend defined limits, of course). Until then we should try to modify
persistent grants not too much IMO.


Juergen