From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756345AbcGGK45 (ORCPT ); Thu, 7 Jul 2016 06:56:57 -0400 Received: from smtp02.citrix.com ([66.165.176.63]:64888 "EHLO SMTP02.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751523AbcGGK44 (ORCPT ); Thu, 7 Jul 2016 06:56:56 -0400 X-IronPort-AV: E=Sophos;i="5.28,324,1464652800"; d="scan'208";a="371753399" Date: Thu, 7 Jul 2016 12:56:54 +0200 From: Roger Pau Monne To: Jan Beulich CC: , Konrad Rzeszutek Wilk , Subject: Re: [PATCH] xen-blkfront: avoid NULL de-reference in CDROM ioctl handling Message-ID: <20160707105654.irp7h7uk5fttbgvb@mac> References: <577E23A602000078000FBE15@prv-mh.provo.novell.com> <20160707093211.dn5mrvgmavpd2mul@mac> <577E400A02000078000FC0CB@prv-mh.provo.novell.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <577E400A02000078000FC0CB@prv-mh.provo.novell.com> User-Agent: Mutt/1.6.0-neo (2016-04-07) X-DLP: MIA1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jul 07, 2016 at 03:42:02AM -0600, Jan Beulich wrote: > >>> On 07.07.16 at 11:32, wrote: > > On Thu, Jul 07, 2016 at 01:40:54AM -0600, Jan Beulich wrote: > >> The ioctl can be called prior to full device setup having completed. > >> > >> Signed-off-by: Jan Beulich > >> --- > >> drivers/block/xen-blkfront.c | 6 ++---- > >> 1 file changed, 2 insertions(+), 4 deletions(-) > >> > >> --- 4.7-rc6-xen.orig/drivers/block/xen-blkfront.c > >> +++ 4.7-rc6-xen/drivers/block/xen-blkfront.c > >> @@ -496,12 +496,10 @@ static int blkif_ioctl(struct block_devi > >> return -EFAULT; > >> return 0; > >> > >> - case CDROM_GET_CAPABILITY: { > >> - struct gendisk *gd = info->gd; > >> - if (gd->flags & GENHD_FL_CD) > >> + case CDROM_GET_CAPABILITY: > >> + if (info->gd && (info->gd->flags & GENHD_FL_CD)) > > > > I don't follow how blkif_ioctl can be called with a NULL info->gd, because > > the set of file operations is set inside of info->gd->fops. And the disk > > should not be available until add_disk is called, which happens after having > > info->gd already set. > > Well, this (as many of the other patches sent earlier today) is a > result of me finally doing a sweep over the old 2.6.18 tree to > see which changes never made it upstream. This is one of them. > Hence I can't entirely exclude that the issue cannot be observed > on a modern kernel (for whatever reason), but it surely was > observed years ago on that old kernel. The sequence of events > in xlvbd_alloc_gendisk() alone certainly leaves room for this, but > I agree that the call to add_disk() _should_ be a prereq to getting > into blkif_ioctl() and definitely only happens after > xlvbd_alloc_gendisk() completed. But I guess you agree that this > _should_ should also have applied to 2.6.18. I agree that it looks like 2.6.18 has the same flow, so that info->gd cannot be NULL if the ioctl handler is called, but without anyone having reported any issue and blkfront code being in upstream Linux for quite a long time I'm reluctant to take this, even more without a trace that shows the actual bug. Roger.