Re: [PATCH 2/3] linux-user: Fix unlock_user API usage

[Nicholas Piggin <npiggin@gmail.com>]

On Sun, Mar 22, 2026 at 02:54:31PM +0800, Chao Liu wrote:
> On Sat, Mar 21, 2026 at 10:48:35AM +1000, Nicholas Piggin wrote:
> > Fix errors in unlock_user() calls:
> > - unlock_user() with len=1 instead of len=written
> > - unlock_user() with len=1 instead of len=0
> > - unlock_user() with len=0 instead of len=1
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  linux-user/linuxload.c | 2 +-
> >  linux-user/syscall.c   | 6 +++---
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/linux-user/linuxload.c b/linux-user/linuxload.c
> > index 85d700953e..79416a94c9 100644
> > --- a/linux-user/linuxload.c
> > +++ b/linux-user/linuxload.c
> > @@ -19,7 +19,7 @@ abi_long memcpy_to_target(abi_ulong dest, const void *src, unsigned long len)
> >          return -TARGET_EFAULT;
> >      }
> >      memcpy(host_ptr, src, len);
> > -    unlock_user(host_ptr, dest, 1);
> > +    unlock_user(host_ptr, dest, len);
> This fixes the writeback -- old code had 0 which meant
> no flush at all, so getsockopt results were silently
> lost under CONFIG_DEBUG_REMAP.
> 
> Minor observation: getsockopt returns actual bytes in
> lv, so using lv rather than len would be more precise.
> That said, the swap loop above also uses len, so this
> is consistent with the existing code.

I guess this comment is for the next hunk below.

The swap loop as you noted stores to the full len, so I
think it's better to use that. If the logic changes to
only store lv, then the unlock would change.

> >      return 0;
> >  }
> >  
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 7832a1aba5..13b8bd9ed3 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -2989,7 +2989,7 @@ get_timeout:
> >              if (put_user_u32(lv, optlen)) {
> >                  return -TARGET_EFAULT;
> >              }
> > -            unlock_user(results, optval_addr, 0);
> > +            unlock_user(results, optval_addr, len);
> >              break;
> >          }

Actually missing unlock on error too, I'll fix that up.

> >  #endif /* LINUX_VERSION_CODE >= KERNEL_VERSION(4, 2, 0) */
> > @@ -4006,7 +4006,7 @@ static inline abi_long host_to_target_semarray(int semid, abi_ulong target_addr,
> >          __put_user((*host_array)[i], &array[i]);
> >      }
> >      g_free(*host_array);
> > -    unlock_user(array, target_addr, 1);
> > +    unlock_user(array, target_addr, nsems * sizeof(unsigned short));
> >  
> >      return 0;
> >  }
> > @@ -7888,7 +7888,7 @@ static inline abi_long target_to_host_sigevent(struct sigevent *host_sevp,
> >      host_sevp->sigev_notify = tswap32(target_sevp->sigev_notify);
> >      host_sevp->sigev_notify_thread_id = tswap32(target_sevp->_sigev_un._tid);
> >  
> > -    unlock_user_struct(target_sevp, target_addr, 1);
> > +    unlock_user_struct(target_sevp, target_addr, 0);
> Right. This function only reads from target, so
> copy=0 (no writeback) is the correct semantic.
> 
> Reviewed-by: Chao Liu <chao.liu.zevorn@gmail.com>

Thanks,
Nick