Re: [Qemu-devel] [PATCH] linux-user: Use *at functions to implement interp_prefix

[Richard Henderson]

On 02/13/2018 04:12 AM, Peter Maydell wrote:
>> +const char *linux_user_path(const char *pathname)
>> +{
>> +    static THREAD size_t save_len;
>> +    static THREAD char *save_buf;
> 
> I think THREAD is a remnant of when we used to support configuration
> with and without NPTL, so we should be looking to get rid of it,
> and just use __thread instead.

Fair.

>> +    size_t len, prefix_len, path_len;
>> +    int e;
>> +
>> +    /* Only consider absolute paths.  */
>> +    if (pathname[0] != '/' || interp_dirfd < 0) {
>> +        return pathname;
>> +    }
>> +
>> +    /* Test if the path within interp_dir exists.  */
>> +    e = faccessat(interp_dirfd, pathname + 1, F_OK, AT_SYMLINK_NOFOLLOW);
>> +    if (e < 0 && errno != ENOENT) {
>> +        return pathname;
>> +    }
>> +
>> +    /* It does -- form the new absolute path.  */
>> +    prefix_len = strlen(interp_prefix);
>> +    path_len = strlen(pathname) + 1;
>> +    len = prefix_len + path_len;
>> +    if (len <= save_len) {
>> +        save_len = len;
>> +        save_buf = realloc(save_buf, len);
>> +    }
>> +    memcpy(save_buf, interp_prefix, prefix_len);
>> +    memcpy(save_buf + prefix_len, pathname, path_len);
>> +
>> +    return save_buf;
>> +}
> 
> The split of an atomic syscall into a two-part thing involving
> an existence/access check and then the syscall makes me a bit
> nervous about races, but I guess there's nothing terrible that
> would happen here...

It's no different from what we have now except that currently the existence
check is done *much* earlier.

> I had a look at where we still need this function:
>  * NR_acct
>  * NR_statfs (various flavours)
>  * NR_inotify_add_watch
> 
> For inotify_add_watch, we can first try the syscall on the
> path inside the interp_dir, and if that fails ENOENT then
> try it with the normal path.

Fair.

>>      /* Scan interp_prefix dir for replacement files. */
>> -    init_paths(interp_prefix);
>> +    interp_dirfd = open(interp_prefix, O_CLOEXEC | O_DIRECTORY | O_PATH);
>>
>>      init_qemu_uname_release();
> 
> I wonder if there are guest programs that make assumptions about
> file descriptor numbers such that it would be worthwhile dup2'ing
> the interp_dirfd away from the presumably low number fd it will
> get by default into something larger...

Hmm.  Using dup2(probe, probe) to test if the new (high) fd itself has not been
allocated?

> Have you tried an LTP test run with this patchset to see whether
> we get any new passes/fails ?

No.  All I've really done is point -L at a debian rootfs and see that mainline
qemu is unusable in that state.


r~