Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <711135296d62cfa0d5dee744a0de86141d57cd6d.camel@linux.ibm.com>
Subject: Re: [PATCH] s390/pci: remove custom and misleading bitmap_vzalloc
From:   Niklas Schnelle <schnelle@linux.ibm.com>
To:     Vasily Gorbik <gor@linux.ibm.com>,
        Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Heiko Carstens <hca@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        linux-kernel@vger.kernel.org, linux-s390@vger.kernel.org
Date:   Mon, 23 Oct 2023 18:31:30 +0200
In-Reply-To: <your-ad-here.call-01698077514-ext-9164@work.hours>
References: <your-ad-here.call-01697881440-ext-2458@work.hours>
         <CAHk-=wgTUz1bdY6zvsN4ED0arCLE8Sb==1GH8d0sjm5bu7zesQ@mail.gmail.com>
         <your-ad-here.call-01698077344-ext-9104@work.hours>
         <your-ad-here.call-01698077514-ext-9164@work.hours>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Precedence: bulk

On Mon, 2023-10-23 at 18:11 +0200, Vasily Gorbik wrote:
> This commit effectively reverts commit c1ae1c59c8c6 ("s390/pci: fix
> iommu bitmap allocation") and applies a simpler fix instead. Commit
> c1ae1c59c8c6 introduced a custom bitmap_vzalloc() function that included
> unnecessary and misleading overflow handling.
>=20
> This fix is only relevant for the current v6.6 and stable backports. It
> will be superseded by the upcoming conversion to use the common
> code DMA API on s390 (pending in linux-next [2]), which eliminates
> arch/s390/pci/pci_dma.c entirely and, therefore, addresses the original
> problem in another way.
>=20
> Instead of relying on a custom bitmap_vzalloc() function, this change goes
> back to straightforward allocation using vzalloc() with the appropriate
> size calculated using the BITS_TO_LONGS() macro.
>=20
> Link: https://lore.kernel.org/all/CAHk-=3DwgTUz1bdY6zvsN4ED0arCLE8Sb=3D=
=3D1GH8d0sjm5bu7zesQ@mail.gmail.com/
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git=
/commit/?h=3Dnext-20231020&id=3Dc76c067e488c
> Cc: stable@vger.kernel.org
> Fixes: c1ae1c59c8c6 ("s390/pci: fix iommu bitmap allocation")
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>
> ---
>  arch/s390/pci/pci_dma.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
>=20
> diff --git a/arch/s390/pci/pci_dma.c b/arch/s390/pci/pci_dma.c
> index 99209085c75b..1b4b123d79aa 100644
> --- a/arch/s390/pci/pci_dma.c
> +++ b/arch/s390/pci/pci_dma.c
> @@ -565,17 +565,6 @@ static void s390_dma_unmap_sg(struct device *dev, st=
ruct scatterlist *sg,
>  	}
>  }
>=20=20
> -static unsigned long *bitmap_vzalloc(size_t bits, gfp_t flags)
> -{
> -	size_t n =3D BITS_TO_LONGS(bits);
> -	size_t bytes;
> -
> -	if (unlikely(check_mul_overflow(n, sizeof(unsigned long), &bytes)))
> -		return NULL;
> -
> -	return vzalloc(bytes);
> -}
> -=09
>  int zpci_dma_init_device(struct zpci_dev *zdev)
>  {
>  	u8 status;
> @@ -615,13 +604,15 @@ int zpci_dma_init_device(struct zpci_dev *zdev)
>  				zdev->end_dma - zdev->start_dma + 1);
>  	zdev->end_dma =3D zdev->start_dma + zdev->iommu_size - 1;
>  	zdev->iommu_pages =3D zdev->iommu_size >> PAGE_SHIFT;
> -	zdev->iommu_bitmap =3D bitmap_vzalloc(zdev->iommu_pages, GFP_KERNEL);
> +	zdev->iommu_bitmap =3D vzalloc(BITS_TO_LONGS(zdev->iommu_pages) *
> +				     sizeof(unsigned long));
>  	if (!zdev->iommu_bitmap) {
>  		rc =3D -ENOMEM;
>  		goto free_dma_table;
>  	}
>  	if (!s390_iommu_strict) {
> -		zdev->lazy_bitmap =3D bitmap_vzalloc(zdev->iommu_pages, GFP_KERNEL);
> +		zdev->lazy_bitmap =3D vzalloc(BITS_TO_LONGS(zdev->iommu_pages) *
> +					    sizeof(unsigned long));
>  		if (!zdev->lazy_bitmap) {
>  			rc =3D -ENOMEM;
>  			goto free_bitmap;

Mea culpa for the useless and misleading overflow check. I'm sorry, I
should not have copied this over from kvmalloc_array() without actually
thinking through whether it makes sense in the new place and you're
right Linus it doesn't.

Thank you Vasily for cleaning this up! Also as an additional point,
note that the size of the bitmap is limited by the above min3() which
in the largest possible case ensures a maximum of 128 MiB bitmaps which
only happens for very large memory systems or if a user sets an
unreasonably large s390_iommu_aperture kernel parameter.

Also:
Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>

Best regards,
Niklas