Re: [PATCH 03/10] vsock: owner field is specific to VMCI

[Jorgen Hansen <jhansen@vmware.com>]

> On 26 Oct 2021, at 13:16, Stefano Garzarella <sgarzare@redhat.com> wrote:
> 
> CCing Jorgen.
> 
> On Thu, Oct 21, 2021 at 04:37:07PM +0400, Marc-André Lureau wrote:
>> This field isn't used by other transports.
> 
> If the field is used only in the VMCI transport, maybe it's better to 
> move the field and the code in that transport.

If the transport needs initialize these fields, that should happen when we
call vsock_assign_transport. So we would need to validate that
get_current_cred() gets the right credentials and that the parent of a
socket has an Initialised owner field at that point in time.

sock_assign_transport may be called when processing an
incoming packet when a remote connects to a listening socket,
and in that case, the owner will be based on the parent socket.
If the parent socket hasn’t been assigned a transport (and as I
remember it, that isn’t the case for a listening socket), then it
isn’t possible to initialize the owner field at this point using
the value from the parent. So the initialisation of the fields
probably have to stay in af_vsock.c as part of the generic structure.

Is there a particular reason to do this change as part of this series
of patches?

Thanks,
Jorgen

> Thanks,
> Stefano
> 
>> 
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>> include/net/af_vsock.h   | 2 ++
>> net/vmw_vsock/af_vsock.c | 6 ++++++
>> 2 files changed, 8 insertions(+)
>> 
>> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>> index ab207677e0a8..e626d9484bc5 100644
>> --- a/include/net/af_vsock.h
>> +++ b/include/net/af_vsock.h
>> @@ -41,7 +41,9 @@ struct vsock_sock {
>>                                       * cached peer?
>>                                       */
>>      u32 cached_peer;  /* Context ID of last dgram destination check. */
>> +#if IS_ENABLED(CONFIG_VMWARE_VMCI_VSOCKETS)
>>      const struct cred *owner;
>> +#endif
>>      /* Rest are SOCK_STREAM only. */
>>      long connect_timeout;
>>      /* Listening socket that this came from. */
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index e2c0cfb334d2..1925682a942a 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -761,7 +761,9 @@ static struct sock *__vsock_create(struct net *net,
>>      psk = parent ? vsock_sk(parent) : NULL;
>>      if (parent) {
>>              vsk->trusted = psk->trusted;
>> +#if IS_ENABLED(CONFIG_VMWARE_VMCI_VSOCKETS)
>>              vsk->owner = get_cred(psk->owner);
>> +#endif
>>              vsk->connect_timeout = psk->connect_timeout;
>>              vsk->buffer_size = psk->buffer_size;
>>              vsk->buffer_min_size = psk->buffer_min_size;
>> @@ -769,7 +771,9 @@ static struct sock *__vsock_create(struct net *net,
>>              security_sk_clone(parent, sk);
>>      } else {
>>              vsk->trusted = ns_capable_noaudit(&init_user_ns, CAP_NET_ADMIN);
>> +#if IS_ENABLED(CONFIG_VMWARE_VMCI_VSOCKETS)
>>              vsk->owner = get_current_cred();
>> +#endif
>>              vsk->connect_timeout = VSOCK_DEFAULT_CONNECT_TIMEOUT;
>>              vsk->buffer_size = VSOCK_DEFAULT_BUFFER_SIZE;
>>              vsk->buffer_min_size = VSOCK_DEFAULT_BUFFER_MIN_SIZE;
>> @@ -833,7 +837,9 @@ static void vsock_sk_destruct(struct sock *sk)
>>      vsock_addr_init(&vsk->local_addr, VMADDR_CID_ANY, VMADDR_PORT_ANY);
>>      vsock_addr_init(&vsk->remote_addr, VMADDR_CID_ANY, VMADDR_PORT_ANY);
>> 
>> +#if IS_ENABLED(CONFIG_VMWARE_VMCI_VSOCKETS)
>>      put_cred(vsk->owner);
>> +#endif
>> }
>> 
>> static int vsock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
>> --
>> 2.33.0.721.g106298f7f9
>> 
>