diff: uvm: fix unitialized var and simplify code in km_alloc()

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

diff: uvm: fix unitialized var and simplify code in km_alloc()

Jan Klemkow
Hi,

The function km_alloc() returns the uninitialized local variable sva if
pgl is empty.  It seems to be not possible in the current condition of
the code, but I'm not sure if this is guaranteed.  Thus, I would prefer
to initialize sva with zero.

It also seems to be unnecessary to loop over the whole pagelist to find
the first element.  The marco pmap_map_direct() just does some
calculations and the value of va is discarded.

I build and run the code on amd64 without any issue and regress/sys/uvm
also doesn't show any problems with that diff.

OK?

bye,
Jan

Index: uvm/uvm_km.c
===================================================================
RCS file: /cvs/src/sys/uvm/uvm_km.c,v
retrieving revision 1.136
diff -u -p -r1.136 uvm_km.c
--- uvm/uvm_km.c 18 Feb 2020 12:13:40 -0000 1.136
+++ uvm/uvm_km.c 20 May 2020 06:17:41 -0000
@@ -816,7 +816,7 @@ km_alloc(size_t sz, const struct kmem_va
  paddr_t pla_align;
  int pla_flags;
  int pla_maxseg;
- vaddr_t va, sva;
+ vaddr_t va, sva = 0;
 
  KASSERT(sz == round_page(sz));
 
@@ -851,11 +851,8 @@ km_alloc(size_t sz, const struct kmem_va
  * allocations.
  */
  if (kv->kv_singlepage || kp->kp_maxseg == 1) {
- TAILQ_FOREACH(pg, &pgl, pageq) {
- va = pmap_map_direct(pg);
- if (pg == TAILQ_FIRST(&pgl))
- sva = va;
- }
+ if ((pg = TAILQ_FIRST(&pgl)) != NULL)
+ sva = pmap_map_direct(pg);
  return ((void *)sva);
  }
 #endif

Reply | Threaded
Open this post in threaded view
|

Re: diff: uvm: fix unitialized var and simplify code in km_alloc()

Alexander Bluhm
On Wed, May 20, 2020 at 11:44:57AM +0200, Jan Klemkow wrote:
> The function km_alloc() returns the uninitialized local variable sva if
> pgl is empty.  It seems to be not possible in the current condition of
> the code, but I'm not sure if this is guaranteed.  Thus, I would prefer
> to initialize sva with zero.

I think initializing sva to 0 is better than returning some stack
memory if something goes wrong.

> It also seems to be unnecessary to loop over the whole pagelist to find
> the first element.  The marco pmap_map_direct() just does some
> calculations and the value of va is discarded.

It is not a macro on other architectures.  You must make sure that
it has no side effects everywhere.  I would not touch this.

> I build and run the code on amd64 without any issue and regress/sys/uvm
> also doesn't show any problems with that diff.

Only testing amd64 is not enough for such a diff.

bluhm

> Index: uvm/uvm_km.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_km.c,v
> retrieving revision 1.136
> diff -u -p -r1.136 uvm_km.c
> --- uvm/uvm_km.c 18 Feb 2020 12:13:40 -0000 1.136
> +++ uvm/uvm_km.c 20 May 2020 06:17:41 -0000
> @@ -816,7 +816,7 @@ km_alloc(size_t sz, const struct kmem_va
>   paddr_t pla_align;
>   int pla_flags;
>   int pla_maxseg;
> - vaddr_t va, sva;
> + vaddr_t va, sva = 0;
>
>   KASSERT(sz == round_page(sz));
>
> @@ -851,11 +851,8 @@ km_alloc(size_t sz, const struct kmem_va
>   * allocations.
>   */
>   if (kv->kv_singlepage || kp->kp_maxseg == 1) {
> - TAILQ_FOREACH(pg, &pgl, pageq) {
> - va = pmap_map_direct(pg);
> - if (pg == TAILQ_FIRST(&pgl))
> - sva = va;
> - }
> + if ((pg = TAILQ_FIRST(&pgl)) != NULL)
> + sva = pmap_map_direct(pg);
>   return ((void *)sva);
>   }
>  #endif