Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2] xen-blkback: fix compatibility bug with single page
 rings
To:     Paul Durrant <paul@xen.org>, xen-devel@lists.xenproject.org,
        linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     Paul Durrant <pdurrant@amazon.com>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= <roger.pau@citrix.com>,
        Jens Axboe <axboe@kernel.dk>
References: <20210128130441.11744-1-paul@xen.org>
From:   Dongli Zhang <dongli.zhang@oracle.com>
Message-ID: <c3a476c5-c671-4429-73d5-0bf7ced1a06b@oracle.com>
Date:   Thu, 28 Jan 2021 22:20:55 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.1
In-Reply-To: <20210128130441.11744-1-paul@xen.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from [IPv6:2601:646:c303:6700::a4a4] (2601:646:c303:6700::a4a4) by CH2PR08CA0005.namprd08.prod.outlook.com (2603:10b6:610:5a::15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3805.16 via Frontend Transport; Fri, 29 Jan 2021 06:20:58 +0000
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: 330118b8-1fbd-4732-dbbc-08d8c41e0b75
X-MS-TrafficTypeDiagnostic: SJ0PR10MB4784:
X-MS-Exchange-Transport-Forked: True
X-Microsoft-Antispam-PRVS: <SJ0PR10MB4784C48C3BAA37442BB11676F0B99@SJ0PR10MB4784.namprd10.prod.outlook.com>
X-MS-Oob-TLC-OOBClassifiers: OLM:114;
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: fBTLHXlLvdITXJAI9wG+Fw5zJ7cyg7gtYjm5zdEbeM2yVrkfE+ilXppXw3FOS6RwD6ce1Py1i9IbqeFeDiZzxIk83rZf0D72CKBAQyR00LIUOB7FwIxIUB9v5M/hnnJjlGa9O/yDLUkdAiHCH+lrnz3pX68dDbGxCy0XN6Z6y25K1BAy8SbIm2vx0miyxenrXdWhwXdsy5fUNOBROA0s3jwlvzJKM+VEsdCgn/C6Mn8wJkje3nWSWuKkphYqz895MeKkRmeaPg5+pRuWVEu/cwnfdLJRl63PnqvEK6lqyoj/zfdGMgqbshDUKWWpw2tWEpXRCV7F1lm9QET9hLWBV3yDlQukw84LQSvm8HwNdIC1MM42pQN67eHBqRZJnf60eDghQhRbs2usfvVodOk3btbAUKEE7nHsirEMMDEaSQpmmEER+AplV8lMqG6xBgDPES/2ZnatpIdJGX+PuW29JzmSKH76A3wj7YcHIicLByKAyK+PvaIDpJGSlbyWYDpplNdSe0HEvqCOucyUt6HrRJXcJziJJXCx9w7q1rbt0e5Ucb3IG/BE0J8HnL7JGGy+DblJBFbSL3Fh324WJ7YcJsytz1CDiap4QSDvgmtxZoE=
X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BYAPR10MB2663.namprd10.prod.outlook.com;PTR:;CAT:NONE;SFS:(346002)(396003)(366004)(39860400002)(136003)(376002)(86362001)(16526019)(8676002)(31696002)(186003)(6486002)(44832011)(5660300002)(83380400001)(478600001)(53546011)(54906003)(36756003)(316002)(66556008)(4326008)(8936002)(66476007)(66946007)(6666004)(31686004)(2616005)(2906002)(43740500002)(45980500001);DIR:OUT;SFP:1101;
X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?eDU3UC8wM0Q2TGs4cFJVeVlqbDdFTnRNMXV0RjV4c1pRNXdWQjJRM2MvL1JP?=
 =?utf-8?B?YUl5eGlBaTRiaHNKWGtHM05DSVJqUDduVXVrbTFWdFlOQXNzNUgzK2RIVXJO?=
 =?utf-8?B?UEIyekNwcExZMmVzdUFYK2RGZEtZeFlmY2hIZUxZa0QrQ0hMMU5SYXFGK1hs?=
 =?utf-8?B?MGdtcmpxV256YUlJZlp6QVV6RGtNTjI3VUZBc0ZCOS9HdjF2TTRGSlAxSFBt?=
 =?utf-8?B?c05aWHA1bUkwdHZYRGxmNnlCWW1aV0x6SDlMZ0c2aUhHWXhFNVBHazA4SVBy?=
 =?utf-8?B?QTcyeXN2c2lNeHBONGYrT3hVeWdrU29RMFZKd0hxT3Y4OHNDVEphdWF3UTZF?=
 =?utf-8?B?eUtLcTJMbmg0ZXNtNURHdE1MSWh0elBCOEJ1TkFLazVNbUlFUDFBN1dRNDc5?=
 =?utf-8?B?akZRS3lCZmFocUpSZU5SM3JrSDlXRlNFc1JNOUtDR3R0bjYzLzlOMEp2b0d5?=
 =?utf-8?B?TlRFVmlOMFlNTlk5LzlxQnlISEFpRnBmREZrSjB3dWhEMEdqeWRqUitKaTFF?=
 =?utf-8?B?VHU1cDk0SytVakhqWTloT2poeXpZMFVsbmJ1eU9NVGVkOGF4N2VGc1lCcmg0?=
 =?utf-8?B?cGU2bm5uWlZMdTQ1RGE0cC9yTWxLV1JXQXNGN0VLTk8wV0pYUnVvQW5xdUtP?=
 =?utf-8?B?MzlIY3FPUFN2R2lvZWo4cmpmY2ZIVDF6SVNMWVVXWHVTS01LMnBUMnRZRGFx?=
 =?utf-8?B?Ykk1bGJnL3F4WHoyM0NaUjdoNWViMTZoUGdyNG1rYXIyc2Vnck1rcStPK3F3?=
 =?utf-8?B?UmU1anRsY0ZFdGZNTEMyVHFxSWhZbEZvUFdURVJjbHJ2aWk1TGJuVDFPYXVl?=
 =?utf-8?B?RmFsOHV2eTZ4c2JHLzFCSGxaenRtNTdMWnVlQ1M5NnlLandUdUFoRjArK3VO?=
 =?utf-8?B?S0prOFF6YzBRWWFKU1ZtVEdURWt1T1lYcmVMWnNvZVhMM29lTm9WK1MzTXlY?=
 =?utf-8?B?N1B6QytFNkxFbHROTDVqWTMzbWFlMnQ5UUN5WWI1WGxzQ2RGenR3cWhERFF5?=
 =?utf-8?B?K0hORHRHMTU2Q3pKY3ZleVpmUFlQaVQ0MzgrQVdjbldLUFJzVGkwTVQ1MUdK?=
 =?utf-8?B?ZytUTjcwMmR4dVFrSXpzbFhITmlpQmJ1V21vM2ZKYzVrR09OOFhnY3I4NHZM?=
 =?utf-8?B?aDBBSFlrbSs3TnA5OWY0elhHMmRWUDJ3bkYvaU1aSVNXdEs0NGZiNGFDRE83?=
 =?utf-8?B?bVBWc2M4QXBRS2tTVlhKaExtRCtGVXVKcFRUcmFtcVZxVHkvNW9jQmY4b0FR?=
 =?utf-8?B?ZEppOGo5dVdGbllkdGQxaUpXQ2RqU2lNdVVTT0Z3TXdnUXVhUlRsWFNZY1hl?=
 =?utf-8?B?NHBJRjgxUU1NbitISjEwVnVzQW51N05PZGRuRnNLSElDaTNQMUtJQ2VtWkxN?=
 =?utf-8?B?ZXFhNU9sZ0dFTEFhNy9PTDF4YlZrWnJSRUwyT3NONDNLekpHYWxvTDlmN2ZP?=
 =?utf-8?B?ckNUOVNBd0l1K3NtK2w5bUFWdVhkKzVTUW84bUd3PT0=?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 330118b8-1fbd-4732-dbbc-08d8c41e0b75
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB2663.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Jan 2021 06:20:59.5180
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: bB0ycOdfWbwBEuhEhStnjUNlA86er2Bp6GlDEFWZWb/OLB9dkK8ns+8NuLpTeHXwzj/6QYCMbFyxFWtc627SOw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4784
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9878 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 suspectscore=0
 adultscore=0 mlxscore=0 malwarescore=0 spamscore=0 mlxlogscore=999
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2009150000 definitions=main-2101290033
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9878 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 impostorscore=0
 phishscore=0 bulkscore=0 priorityscore=1501 mlxlogscore=999
 lowpriorityscore=0 spamscore=0 mlxscore=0 suspectscore=0 malwarescore=0
 clxscore=1015 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2009150000 definitions=main-2101290033
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 1/28/21 5:04 AM, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> Prior to commit 4a8c31a1c6f5 ("xen/blkback: rework connect_ring() to avoid
> inconsistent xenstore 'ring-page-order' set by malicious blkfront"), the
> behaviour of xen-blkback when connecting to a frontend was:
> 
> - read 'ring-page-order'
> - if not present then expect a single page ring specified by 'ring-ref'
> - else expect a ring specified by 'ring-refX' where X is between 0 and
>   1 << ring-page-order
> 
> This was correct behaviour, but was broken by the afforementioned commit to
> become:
> 
> - read 'ring-page-order'
> - if not present then expect a single page ring (i.e. ring-page-order = 0)
> - expect a ring specified by 'ring-refX' where X is between 0 and
>   1 << ring-page-order
> - if that didn't work then see if there's a single page ring specified by
>   'ring-ref'
> 
> This incorrect behaviour works most of the time but fails when a frontend
> that sets 'ring-page-order' is unloaded and replaced by one that does not
> because, instead of reading 'ring-ref', xen-blkback will read the stale
> 'ring-ref0' left around by the previous frontend will try to map the wrong
> grant reference.
> 
> This patch restores the original behaviour.
> 
> Fixes: 4a8c31a1c6f5 ("xen/blkback: rework connect_ring() to avoid inconsistent xenstore 'ring-page-order' set by malicious blkfront")
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Dongli Zhang <dongli.zhang@oracle.com>
> 
> v2:
>  - Remove now-spurious error path special-case when nr_grefs == 1
> ---
>  drivers/block/xen-blkback/common.h |  1 +
>  drivers/block/xen-blkback/xenbus.c | 38 +++++++++++++-----------------
>  2 files changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
> index b0c71d3a81a0..524a79f10de6 100644
> --- a/drivers/block/xen-blkback/common.h
> +++ b/drivers/block/xen-blkback/common.h
> @@ -313,6 +313,7 @@ struct xen_blkif {
>  
>  	struct work_struct	free_work;
>  	unsigned int 		nr_ring_pages;
> +	bool                    multi_ref;

Is it really necessary to introduce 'multi_ref' here or we may just re-use
'nr_ring_pages'?

According to blkfront code, 'ring-page-order' is set only when it is not zero,
that is, only when (info->nr_ring_pages > 1).

1819         if (info->nr_ring_pages > 1) {
1820                 err = xenbus_printf(xbt, dev->nodename, "ring-page-order",
"%u",
1821                                     ring_page_order);
1822                 if (err) {
1823                         message = "writing ring-page-order";
1824                         goto abort_transaction;
1825                 }
1826         }

Therefore, can we assume 'ring-page-order' can never be 0? Once we have
'ring-page-order' set, it should be >= 1 and we should read from "ring-ref%u"?

If the specification allows 'ring-page-order' to be zero with "ring-ref%u"
available, we should introduce 'multi_ref'.

Thank you very much!

Dongli Zhang


>  	/* All rings for this device. */
>  	struct xen_blkif_ring	*rings;
>  	unsigned int		nr_rings;
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index 9860d4842f36..6c5e9373e91c 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -998,14 +998,17 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
>  	for (i = 0; i < nr_grefs; i++) {
>  		char ring_ref_name[RINGREF_NAME_LEN];
>  
> -		snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
> +		if (blkif->multi_ref)
> +			snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref%u", i);
> +		else {
> +			WARN_ON(i != 0);
> +			snprintf(ring_ref_name, RINGREF_NAME_LEN, "ring-ref");
> +		}
> +
>  		err = xenbus_scanf(XBT_NIL, dir, ring_ref_name,
>  				   "%u", &ring_ref[i]);
>  
>  		if (err != 1) {
> -			if (nr_grefs == 1)
> -				break;
> -
>  			err = -EINVAL;
>  			xenbus_dev_fatal(dev, err, "reading %s/%s",
>  					 dir, ring_ref_name);
> @@ -1013,18 +1016,6 @@ static int read_per_ring_refs(struct xen_blkif_ring *ring, const char *dir)
>  		}
>  	}
>  
> -	if (err != 1) {
> -		WARN_ON(nr_grefs != 1);
> -
> -		err = xenbus_scanf(XBT_NIL, dir, "ring-ref", "%u",
> -				   &ring_ref[0]);
> -		if (err != 1) {
> -			err = -EINVAL;
> -			xenbus_dev_fatal(dev, err, "reading %s/ring-ref", dir);
> -			return err;
> -		}
> -	}
> -
>  	err = -ENOMEM;
>  	for (i = 0; i < nr_grefs * XEN_BLKIF_REQS_PER_PAGE; i++) {
>  		req = kzalloc(sizeof(*req), GFP_KERNEL);
> @@ -1129,10 +1120,15 @@ static int connect_ring(struct backend_info *be)
>  		 blkif->nr_rings, blkif->blk_protocol, protocol,
>  		 blkif->vbd.feature_gnt_persistent ? "persistent grants" : "");
>  
> -	ring_page_order = xenbus_read_unsigned(dev->otherend,
> -					       "ring-page-order", 0);
> -
> -	if (ring_page_order > xen_blkif_max_ring_order) {
> +	err = xenbus_scanf(XBT_NIL, dev->otherend, "ring-page-order", "%u",
> +			   &ring_page_order);
> +	if (err != 1) {
> +		blkif->nr_ring_pages = 1;
> +		blkif->multi_ref = false;
> +	} else if (ring_page_order <= xen_blkif_max_ring_order) {
> +		blkif->nr_ring_pages = 1 << ring_page_order;
> +		blkif->multi_ref = true;
> +	} else {
>  		err = -EINVAL;
>  		xenbus_dev_fatal(dev, err,
>  				 "requested ring page order %d exceed max:%d",
> @@ -1141,8 +1137,6 @@ static int connect_ring(struct backend_info *be)
>  		return err;
>  	}
>  
> -	blkif->nr_ring_pages = 1 << ring_page_order;
> -
>  	if (blkif->nr_rings == 1)
>  		return read_per_ring_refs(&blkif->rings[0], dev->otherend);
>  	else {
>