Re: [PATCH 1/2][PR GDB/32956] gdb: implement linux namespace support for fileio_stat

[Andrew Burgess <aburgess@redhat.com>]

Fabian Kilger <kilger@sec.in.tum.de> writes:

> While looking at it, I've noticed all implementations of stat functions
> actually use lstat and not stat. This maybe should be modified in the
> namespace case as well and I'll be changing this for v2. However, I
> could not directly find a rationale behind why *_stat functions call
> lstat instead. Possibly, it might make sense renaming the
> target_fileio_stat to target_fileio_lstat as well, though this would be
> an independent change.

Without digging into the history I say for sure why it's lstat over
stat.

I agree with you that renaming the API might make some sense, though I
suspect we're really a little stuck, the remote protocol packet is
called 'stat', which is unfortunate, we cannot really change that.

I do agree that within the namespace code we should probably use lstat
in order to be consistent.

Thanks,
Andrew



>
> Best,
> Fabian
>
> On 5/24/25 21:43, Fabian Kilger wrote:
>> Hi Andrew,
>> 
>> you're right, I'm directly using GDB and wasn't aware of that gdbserver
>> module. I'll be adding it to the patch for v2.
>> 
>> Best,
>> Fabian
>> 
>> On 5/24/25 12:46, Andrew Burgess wrote:
>>> Fabian Kilger <kilger@sec.in.tum.de> writes:
>>>
>>>> The new algorithm to look for a build-id-based debug file
>>>> (introduced by commit 22836ca88591ac7efacf06d5b6db191763fd8aba)
>>>> makes use of fileio_stat. As fileio_stat was not supported by
>>>> linux-namespace.c, all stat calls would be performed on the host
>>>> and not inside the namespace
>>>>
>>>> ---
>>>>  gdb/linux-nat.c            | 14 ++++++++
>>>>  gdb/linux-nat.h            |  3 ++
>>>>  gdb/nat/linux-namespaces.c | 71 ++++++++++++++++++++++++++++++++++++++
>>>>  gdb/nat/linux-namespaces.h |  6 ++++
>>>
>>> Thinking about this some more, I realised that this is missing the
>>> gdbserver related changes.
>>>
>>> If you search in the gdbserver/ directory for how multifs_readlink is
>>> used then this will give a pretty good idea for how a new multifs_stat
>>> should be added.
>>>
>>> For manual testing things will be pretty similar to testing GDB.  On the
>>> same host as your container, but outside of the container, start
>>> gdbserver:
>>>
>>>   $ gdbserver --multi --once :54321
>>>
>>> Then on the same host, start GDB, and within GDB:
>>>
>>>   (gdb) target extended-remote :54321
>>>   (gdb) attach PID
>>>
>>> I'm assuming that currently you are just attaching directly from GDB to
>>> a process within the container?
>>>
>>> Thanks,
>>> Andrew
>>>
>>>>  4 files changed, 94 insertions(+)
>>>>
>>>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
>>>> index 3f252370c7b..478a7977c4d 100644
>>>> --- a/gdb/linux-nat.c
>>>> +++ b/gdb/linux-nat.c
>>>> @@ -4585,6 +4585,20 @@ linux_nat_target::fileio_open (struct inferior *inf, const char *filename,
>>>>    return fd;
>>>>  }
>>>>  
>>>> +/* Implementation of to_fileio_stat.  */
>>>> +int
>>>> +linux_nat_target::fileio_stat (struct inferior *inf, const char *filename,
>>>> +			       struct stat *sb, fileio_error *target_errno)
>>>> +{
>>>> +  int r = linux_mntns_stat (linux_nat_fileio_pid_of (inf),
>>>> +			    filename, sb);
>>>> +
>>>> +  if (r == -1)
>>>> +    *target_errno = host_to_fileio_error (errno);
>>>> +
>>>> +  return r;
>>>> +}
>>>> +
>>>>  /* Implementation of to_fileio_readlink.  */
>>>>  
>>>>  std::optional<std::string>
>>>> diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
>>>> index b630b858e34..42d1ec142b3 100644
>>>> --- a/gdb/linux-nat.h
>>>> +++ b/gdb/linux-nat.h
>>>> @@ -108,6 +108,9 @@ class linux_nat_target : public inf_ptrace_target
>>>>  		     const char *filename,
>>>>  		     fileio_error *target_errno) override;
>>>>  
>>>> +  int fileio_stat (struct inferior *inf, const char *filename,
>>>> +		   struct stat *sb, fileio_error *target_errno) override;
>>>> +
>>>>    int fileio_unlink (struct inferior *inf,
>>>>  		     const char *filename,
>>>>  		     fileio_error *target_errno) override;
>>>> diff --git a/gdb/nat/linux-namespaces.c b/gdb/nat/linux-namespaces.c
>>>> index 19a05eec905..aa74e9df950 100644
>>>> --- a/gdb/nat/linux-namespaces.c
>>>> +++ b/gdb/nat/linux-namespaces.c
>>>> @@ -233,6 +233,12 @@ enum mnsh_msg_type
>>>>         MNSH_RET_INT.  */
>>>>      MNSH_REQ_SETNS,
>>>>  
>>>> +    /* A request that the helper call stat.  The single
>>>> +       argument (the filename) should be passed in BUF, and
>>>> +       should include a terminating NUL character.  The helper
>>>> +       should respond with a MNSH_RET_INTSTR.  */
>>>> +    MNSH_REQ_STAT,
>>>> +
>>>>      /* A request that the helper call open.  Arguments should
>>>>         be passed in BUF, INT1 and INT2.  The filename (in BUF)
>>>>         should include a terminating NUL character.  The helper
>>>> @@ -283,6 +289,10 @@ mnsh_debug_print_message (enum mnsh_msg_type type,
>>>>        debug_printf ("ERROR");
>>>>        break;
>>>>  
>>>> +    case MNSH_REQ_STAT:
>>>> +      debug_printf ("STAT");
>>>> +      break;
>>>> +
>>>>      case MNSH_REQ_SETNS:
>>>>        debug_printf ("SETNS");
>>>>        break;
>>>> @@ -514,6 +524,20 @@ mnsh_handle_setns (int sock, int fd, int nstype)
>>>>    return mnsh_return_int (sock, result, errno);
>>>>  }
>>>>  
>>>> +
>>>> +/* Handle a MNSH_REQ_STAT message.  Must be async-signal-safe.  */
>>>> +
>>>> +static ssize_t
>>>> +mnsh_handle_stat(int sock, const char *filename)
>>>> +{
>>>> +  struct stat sb;
>>>> +  int stat_ok = stat(filename, &sb);
>>>> +
>>>> +  return mnsh_return_intstr(sock, stat_ok, &sb,
>>>> +			    stat_ok == -1 ? 0 : sizeof (sb),
>>>> +			    errno);
>>>> +}
>>>> +
>>>>  /* Handle a MNSH_REQ_OPEN message.  Must be async-signal-safe.  */
>>>>  
>>>>  static ssize_t
>>>> @@ -574,6 +598,11 @@ mnsh_main (int sock)
>>>>  		response = mnsh_handle_setns (sock, fd, int1);
>>>>  	      break;
>>>>  
>>>> +	    case MNSH_REQ_STAT:
>>>> +	      if (size > 0 && buf[size - 1] == '\0')
>>>> +		response = mnsh_handle_stat(sock, buf);
>>>> +	      break;
>>>> +
>>>>  	    case MNSH_REQ_OPEN:
>>>>  	      if (size > 0 && buf[size - 1] == '\0')
>>>>  		response = mnsh_handle_open (sock, buf, int1, int2);
>>>> @@ -765,6 +794,10 @@ mnsh_maybe_mourn_peer (void)
>>>>    mnsh_send_message (helper->sock, MNSH_REQ_OPEN, -1, flags, mode, \
>>>>  		     filename, strlen (filename) + 1)
>>>>  
>>>> +#define mnsh_send_stat(helper, filename) \
>>>> +  mnsh_send_message (helper->sock, MNSH_REQ_STAT, -1, 0, 0, \
>>>> +		     filename, strlen (filename) + 1)
>>>> +
>>>>  #define mnsh_send_unlink(helper, filename) \
>>>>    mnsh_send_message (helper->sock, MNSH_REQ_UNLINK, -1, 0, 0, \
>>>>  		     filename, strlen (filename) + 1)
>>>> @@ -945,6 +978,44 @@ linux_mntns_access_fs (pid_t pid)
>>>>    return MNSH_FS_HELPER;
>>>>  }
>>>>  
>>>> +
>>>> +/* See nat/linux-namespaces.h.  */
>>>> +int
>>>> +linux_mntns_stat (pid_t pid, const char *filename,
>>>> +		  struct stat *sb)
>>>> +{
>>>> +  enum mnsh_fs_code access = linux_mntns_access_fs (pid);
>>>> +  struct linux_mnsh *helper;
>>>> +  int stat_ok, error;
>>>> +  ssize_t size;
>>>> +
>>>> +  if (access == MNSH_FS_ERROR)
>>>> +    return -1;
>>>> +
>>>> +  if (access == MNSH_FS_DIRECT)
>>>> +    return stat(filename, sb);
>>>> +
>>>> +  gdb_assert (access == MNSH_FS_HELPER);
>>>> +
>>>> +  helper = linux_mntns_get_helper ();
>>>> +
>>>> +  size = mnsh_send_stat (helper, filename);
>>>> +  if (size < 0)
>>>> +    return -1;
>>>> +
>>>> +  size = mnsh_recv_intstr (helper, &stat_ok, &error, sb, sizeof (*sb));
>>>> +
>>>> +  if (size < 0)
>>>> +    {
>>>> +      stat_ok = -1;
>>>> +      errno = error;
>>>> +    }
>>>> +  else
>>>> +    gdb_assert (stat_ok == -1 || size == sizeof (*sb));
>>>> +
>>>> +  return stat_ok;
>>>> +}
>>>> +
>>>>  /* See nat/linux-namespaces.h.  */
>>>>  
>>>>  int
>>>> diff --git a/gdb/nat/linux-namespaces.h b/gdb/nat/linux-namespaces.h
>>>> index 4327292950b..825cb27eb2d 100644
>>>> --- a/gdb/nat/linux-namespaces.h
>>>> +++ b/gdb/nat/linux-namespaces.h
>>>> @@ -58,6 +58,12 @@ enum linux_ns_type
>>>>  
>>>>  extern int linux_ns_same (pid_t pid, enum linux_ns_type type);
>>>>  
>>>> +/* Like stat(2), but in the mount namespace of process
>>>> +   PID.  */
>>>> +
>>>> +extern int linux_mntns_stat (pid_t pid, const char *filename,
>>>> +			     struct stat *sb);
>>>> +
>>>>  /* Like gdb_open_cloexec, but in the mount namespace of process
>>>>     PID.  */
>>>>  
>>>> -- 
>>>> 2.49.0
>>>
>> 
>
> -- 
> Fabian Kilger, M.Sc.
> Wissenschaftlicher Mitarbeiter
>
> Technische Universität München
> TUM School of Computation, Information and Technology
> Chair of IT Security
>
> Boltzmannstraße 3
> 85748 Garching (bei München)
>
> Tel. +49 (0)89 289-18587
> Fax +49 (0)89 289-18579
>
> kilger@sec.in.tum.de
> www.sec.in.tum.de
> -----BEGIN PGP PUBLIC KEY BLOCK-----
>
> xjMEYHltfxYJKwYBBAHaRw8BAQdA7mzpLUfZIcIiMjdx+GBa8RuqZdMp/MUEpu4P
> DTb2YwXNJEZhYmlhbiBLaWxnZXIgPGtpbGdlckBzZWMuaW4udHVtLmRlPsKLBBMW
> CAAzFiEETPRi+vRLaNymGJvYr2lqRpshfmkFAmB5bX8CGwMFCwkIBwIGFQgJCgsC
> BRYCAwEAAAoJEK9pakabIX5pCzcA/ivCFRRbxJfpiwOzV5CvflcHPNN2LmCxSBlc
> rBpliBhWAP43PcAtWheftijoLpcwy3nD0TVTDRrJY/hRkKDbvmrWCM44BGB5bX8S
> CisGAQQBl1UBBQEBB0BtYlZed2qkwQWmV+MaUhC78XgZI0ezLuU2nr8bocqXCAMB
> CAfCeAQYFggAIBYhBEz0Yvr0S2jcphib2K9pakabIX5pBQJgeW1/AhsMAAoJEK9p
> akabIX5pUNQA/juajzwCYdtbo+sXQUlZufPiPwLiPr6LuJBNZwL6OlbmAQDvyu6h
> +X9K2gzgLviiNEmcCAddwynvjXiLt3c+oir7AA==
> =VdeZ
> -----END PGP PUBLIC KEY BLOCK-----