Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: iac594737:Fastmail
Date:   Wed, 7 Jun 2023 12:00:29 -0400
From:   Demi Marie Obenour <demi@invisiblethingslab.com>
To:     Christoph Hellwig <hch@infradead.org>
Cc:     Roger Pau =?utf-8?B?TW9ubsOp?= <roger.pau@citrix.com>,
        Jens Axboe <axboe@kernel.dk>,
        Marek =?utf-8?Q?Marczykowski-G=C3=B3recki?= 
        <marmarek@invisiblethingslab.com>, xen-devel@lists.xenproject.org,
        linux-block@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] xen-blkback: Implement diskseq checks
Message-ID: <ZICpodHAGv8qRw5A@itl-email>
References: <20230601214823.1701-1-demi@invisiblethingslab.com>
 <20230601214823.1701-2-demi@invisiblethingslab.com>
 <ZIA1VkDdgt5kmqEt@infradead.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="CTS34Y4jd0AZd9RG"
Content-Disposition: inline
In-Reply-To: <ZIA1VkDdgt5kmqEt@infradead.org>
Precedence: bulk


--CTS34Y4jd0AZd9RG
Content-Type: text/plain; protected-headers=v1; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
Date: Wed, 7 Jun 2023 12:00:29 -0400
From: Demi Marie Obenour <demi@invisiblethingslab.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: Roger Pau =?utf-8?B?TW9ubsOp?= <roger.pau@citrix.com>,
	Jens Axboe <axboe@kernel.dk>,
	Marek =?utf-8?Q?Marczykowski-G=C3=B3recki?= <marmarek@invisiblethingslab.com>,
	xen-devel@lists.xenproject.org, linux-block@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] xen-blkback: Implement diskseq checks

On Wed, Jun 07, 2023 at 12:44:22AM -0700, Christoph Hellwig wrote:
> On Thu, Jun 01, 2023 at 05:48:22PM -0400, Demi Marie Obenour wrote:
> > +	if (diskseq) {
> > +		struct gendisk *disk =3D bdev->bd_disk;
> > +
> > +		if (unlikely(disk =3D=3D NULL)) {
> > +			pr_err("%s: device %08x has no gendisk\n",
> > +			       __func__, vbd->pdevice);
> > +			xen_vbd_free(vbd);
> > +			return -EFAULT;
> > +		}
>=20
> bdev->bd_disk is never NULL.

Fixed in v3.

> > +	diskseq_str =3D xenbus_read(XBT_NIL, dev->nodename, "diskseq", &disks=
eq_len);
>=20
> Please avoid the overly long line.

Fixed in v3.

> > +	if (IS_ERR(diskseq_str)) {
> > +		int err =3D PTR_ERR(diskseq_str);
> > +		diskseq_str =3D NULL;
> > +
> > +		/*
> > +		 * If this does not exist, it means legacy userspace that does not
>=20
> .. even more so in comments.

Fixed in v3.

> > +		 * support diskseq.
> > +		 */
> > +		if (unlikely(!XENBUS_EXIST_ERR(err))) {
> > +			xenbus_dev_fatal(dev, err, "reading diskseq");
> > +			return;
> > +		}
> > +		diskseq =3D 0;
> > +	} else if (diskseq_len <=3D 0) {
> > +		xenbus_dev_fatal(dev, -EFAULT, "diskseq must not be empty");
> > +		goto fail;
> > +	} else if (diskseq_len > 16) {
>=20
> No need for a else after a return.

Fixed in v3.

> > +		xenbus_dev_fatal(dev, -ERANGE, "diskseq too long: got %d but limit i=
s 16",
> > +				 diskseq_len);
> > +		goto fail;
> > +	} else if (diskseq_str[0] =3D=3D '0') {
> > +		xenbus_dev_fatal(dev, -ERANGE, "diskseq must not start with '0'");
> > +		goto fail;
> > +	} else {
> > +		char *diskseq_end;
> > +		diskseq =3D simple_strtoull(diskseq_str, &diskseq_end, 16);
> > +		if (diskseq_end !=3D diskseq_str + diskseq_len) {
> > +			xenbus_dev_fatal(dev, -EINVAL, "invalid diskseq");
> > +			goto fail;
> > +		}
> > +		kfree(diskseq_str);
> > +		diskseq_str =3D NULL;
> > +	}
>=20
> And I suspect the code will be a lot easier to follow if you move
> the diskseq validation into a separate helper.

Fixed in v3.
--=20
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

--CTS34Y4jd0AZd9RG
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEdodNnxM2uiJZBxxxsoi1X/+cIsEFAmSAqaEACgkQsoi1X/+c
IsH5yhAAnK+FHobE4EaJLu/MXl1mYHrYhpZKqUOIopKg1xODd1RlWyQI95qq3vHX
AnwOhHN+Wv/PbJ0vhvL+/DtuC56AIaCxzhShuZmqZjvLDPwdvi73nSf4xiV7SLWG
6GfMhn9Mo8MGoj+ek88P10z9bIwrfdCWnvnuns6bcGlHsMkrx/B3Jzk5ZOD1QWWj
W6jzKRmUDCv+RKHcajLUB3iM2JTTYOLcljqldvSyRv1SySytToLRtTWfZrOJS7Z2
Tjf9heSRPhvHPvcaHyidiaZ87xQjcmI4LRzOXEybxFd6uxIf4QpDD4rschFu25NL
TTEFPRIocqwHQgyVcG8aIu86nizNf7f/O/iLfQhNTuYQ1mbq6l8QNyAcJ6sXMz0A
fms3iMGiH2LF/pi8vEprOfiC1+0L+whpAqEEogIWh4zYp/tM0raENGZNJL6ZTwXI
BPkVkAmsrZAQljrzecbtjRWPZJUDmi+u9XGFCjY9MX7oHtnRH22hLdVeYqd1NljD
bgleEMTPiV/ZhPhOPi/Fyccze9VneSOjQ7QoKii0JaiECzatJFr3ZoIgwpIJsv17
KSglgXTjzQpGPURe+RK+rQWMAlb6V2gT3mNC36txOv8j+Y6CuL3xfwGIdfP5nLh+
qTO2n1i40TxJ0VEZa9kD67tyqrLuOS7kru2hQbdhvW9f92eOPo4=
=m1I6
-----END PGP SIGNATURE-----

--CTS34Y4jd0AZd9RG--