[PATCH v2] tpm: use GFP kernel for tpm_buf allocations

[PATCH v2] tpm: use GFP kernel for tpm_buf allocations

[James Bottomley]

The current code uses GFP_HIGHMEM, which is wrong because GFP_HIGHMEM
(on 32 bit systems) is memory ordinarily inaccessible to the kernel
and should only be used for allocations affecting userspace.  In order
to make highmem visible to the kernel on 32 bit it has to be kmapped,
which consumes valuable entries in the kmap region.  Since the tpm_buf
is only ever used in the kernel, switch to using a GFP_KERNEL
allocation so as not to waste kmap space on 32 bits.

Fixes: a74f8b36352e (tpm: introduce tpm_buf)
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---

v2: fix 0day spotted problem with free_page taking an unsigned long
    not a void *
---
 drivers/char/tpm/tpm.h | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a7fea3e0ca86..3a7998d7309a 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -284,7 +284,6 @@ enum tpm_buf_flags {
 };
 
 struct tpm_buf {
-	struct page *data_page;
 	unsigned int flags;
 	u8 *data;
 };
@@ -300,20 +299,18 @@ static inline void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
 
 static inline int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
 {
-	buf->data_page = alloc_page(GFP_HIGHUSER);
-	if (!buf->data_page)
+	buf->data = (u8 *)__get_free_page(GFP_KERNEL);
+	if (!buf->data)
 		return -ENOMEM;
 
 	buf->flags = 0;
-	buf->data = kmap(buf->data_page);
 	tpm_buf_reset(buf, tag, ordinal);
 	return 0;
 }
 
 static inline void tpm_buf_destroy(struct tpm_buf *buf)
 {
-	kunmap(buf->data_page);
-	__free_page(buf->data_page);
+	free_page((unsigned long)buf->data);
 }
 
 static inline u32 tpm_buf_length(struct tpm_buf *buf)
-- 
2.16.4

Re: [PATCH v2] tpm: use GFP kernel for tpm_buf allocations

[Jarkko Sakkinen]

On Fri, Oct 11, 2019 at 09:02:59AM -0700, James Bottomley wrote:
> The current code uses GFP_HIGHMEM, which is wrong because GFP_HIGHMEM
> (on 32 bit systems) is memory ordinarily inaccessible to the kernel
> and should only be used for allocations affecting userspace.  In order
> to make highmem visible to the kernel on 32 bit it has to be kmapped,
> which consumes valuable entries in the kmap region.  Since the tpm_buf
> is only ever used in the kernel, switch to using a GFP_KERNEL
> allocation so as not to waste kmap space on 32 bits.
> 
> Fixes: a74f8b36352e (tpm: introduce tpm_buf)
> Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

I'll apply this without a fixes tag as there is no failing system.
Agree that it was not the best design decision to use GFP_HIGHMEM.

/Jarkko

Re: [PATCH v2] tpm: use GFP kernel for tpm_buf allocations

[James Bottomley]

On Mon, 2019-10-14 at 22:32 +0300, Jarkko Sakkinen wrote:
> On Fri, Oct 11, 2019 at 09:02:59AM -0700, James Bottomley wrote:
> > The current code uses GFP_HIGHMEM, which is wrong because
> > GFP_HIGHMEM
> > (on 32 bit systems) is memory ordinarily inaccessible to the kernel
> > and should only be used for allocations affecting userspace.  In
> > order
> > to make highmem visible to the kernel on 32 bit it has to be
> > kmapped,
> > which consumes valuable entries in the kmap region.  Since the
> > tpm_buf
> > is only ever used in the kernel, switch to using a GFP_KERNEL
> > allocation so as not to waste kmap space on 32 bits.
> > 
> > Fixes: a74f8b36352e (tpm: introduce tpm_buf)
> > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.c
> > om>
> 
> I'll apply this without a fixes tag as there is no failing system.
> Agree that it was not the best design decision to use GFP_HIGHMEM.

I don't really mind either way.  The function of the fixes tag isn't to
say there was a fatal bug it's to say if you think you're fixing
something where was the origin of the problem, however minor the
problem is.

I do agree that Sasha's autosel stuff does seem to be triggering off
Fixes and we don't want to see hundreds of autosel patches trying to
apply this to older trees, so removing the fixes tag to avoid this is
fine with me.

James