| Message ID | 20201204170235.84387-2-andraprs@amazon.com |
|---|---|
| State | New |
| Headers | show |
| Series | vsock: Add flags field in the vsock address | expand |
On Fri, Dec 04, 2020 at 07:02:32PM +0200, Andra Paraschiv wrote: >vsock enables communication between virtual machines and the host they >are running on. With the multi transport support (guest->host and >host->guest), nested VMs can also use vsock channels for communication. > >In addition to this, by default, all the vsock packets are forwarded to >the host, if no host->guest transport is loaded. This behavior can be >implicitly used for enabling vsock communication between sibling VMs. > >Add a flags field in the vsock address data structure that can be used >to explicitly mark the vsock connection as being targeted for a certain >type of communication. This way, can distinguish between different use >cases such as nested VMs and sibling VMs. > >Use the already available "svm_reserved1" field and mark it as a flags >field instead. This field can be set when initializing the vsock address >variable used for the connect() call. > >Changelog > >v1 -> v2 > >* Update the field name to "svm_flags". >* Split the current patch in 2 patches. Usually the changelog goes after the 3 dashes, but I'm not sure there is a strict rule :-) Anyway the patch LGTM: Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> > >Signed-off-by: Andra Paraschiv <andraprs@amazon.com> >--- > include/uapi/linux/vm_sockets.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h >index fd0ed7221645d..46735376a57a8 100644 >--- a/include/uapi/linux/vm_sockets.h >+++ b/include/uapi/linux/vm_sockets.h >@@ -145,7 +145,7 @@ > > struct sockaddr_vm { > __kernel_sa_family_t svm_family; >- unsigned short svm_reserved1; >+ unsigned short svm_flags; > unsigned int svm_port; > unsigned int svm_cid; > unsigned char svm_zero[sizeof(struct sockaddr) - >-- >2.20.1 (Apple Git-117) > > > > >Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005. >
On 07/12/2020 11:59, Stefano Garzarella wrote: > > On Fri, Dec 04, 2020 at 07:02:32PM +0200, Andra Paraschiv wrote: >> vsock enables communication between virtual machines and the host they >> are running on. With the multi transport support (guest->host and >> host->guest), nested VMs can also use vsock channels for communication. >> >> In addition to this, by default, all the vsock packets are forwarded to >> the host, if no host->guest transport is loaded. This behavior can be >> implicitly used for enabling vsock communication between sibling VMs. >> >> Add a flags field in the vsock address data structure that can be used >> to explicitly mark the vsock connection as being targeted for a certain >> type of communication. This way, can distinguish between different use >> cases such as nested VMs and sibling VMs. >> >> Use the already available "svm_reserved1" field and mark it as a flags >> field instead. This field can be set when initializing the vsock address >> variable used for the connect() call. >> >> Changelog >> >> v1 -> v2 >> >> * Update the field name to "svm_flags". >> * Split the current patch in 2 patches. > > Usually the changelog goes after the 3 dashes, but I'm not sure there is > a strict rule :-) Yup, I've seen both ways. I'd rather keep the changelog in the commit message, for further reference after merge, in the commits. > > Anyway the patch LGTM: > > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> Thank you. Andra > >> >> Signed-off-by: Andra Paraschiv <andraprs@amazon.com> >> --- >> include/uapi/linux/vm_sockets.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/uapi/linux/vm_sockets.h >> b/include/uapi/linux/vm_sockets.h >> index fd0ed7221645d..46735376a57a8 100644 >> --- a/include/uapi/linux/vm_sockets.h >> +++ b/include/uapi/linux/vm_sockets.h >> @@ -145,7 +145,7 @@ >> >> struct sockaddr_vm { >> __kernel_sa_family_t svm_family; >> - unsigned short svm_reserved1; >> + unsigned short svm_flags; >> unsigned int svm_port; >> unsigned int svm_cid; >> unsigned char svm_zero[sizeof(struct sockaddr) - >> -- >> 2.20.1 (Apple Git-117) >> >> >> >> >> Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. >> Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. >> Registered in Romania. Registration number J22/2621/2005. >> > Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
On Fri, 4 Dec 2020 19:02:32 +0200 Andra Paraschiv wrote: > diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h > index fd0ed7221645d..46735376a57a8 100644 > --- a/include/uapi/linux/vm_sockets.h > +++ b/include/uapi/linux/vm_sockets.h > @@ -145,7 +145,7 @@ > > struct sockaddr_vm { > __kernel_sa_family_t svm_family; > - unsigned short svm_reserved1; > + unsigned short svm_flags; > unsigned int svm_port; > unsigned int svm_cid; > unsigned char svm_zero[sizeof(struct sockaddr) - Since this is a uAPI header I gotta ask - are you 100% sure that it's okay to rename this field? I didn't grasp from just reading the patches whether this is a uAPI or just internal kernel flag, seems like the former from the reading of the comment in patch 2. In which case what guarantees that existing users don't pass in garbage since the kernel doesn't check it was 0?
On 07/12/2020 23:29, Jakub Kicinski wrote: > On Fri, 4 Dec 2020 19:02:32 +0200 Andra Paraschiv wrote: >> diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h >> index fd0ed7221645d..46735376a57a8 100644 >> --- a/include/uapi/linux/vm_sockets.h >> +++ b/include/uapi/linux/vm_sockets.h >> @@ -145,7 +145,7 @@ >> >> struct sockaddr_vm { >> __kernel_sa_family_t svm_family; >> - unsigned short svm_reserved1; >> + unsigned short svm_flags; >> unsigned int svm_port; >> unsigned int svm_cid; >> unsigned char svm_zero[sizeof(struct sockaddr) - > Since this is a uAPI header I gotta ask - are you 100% sure that it's > okay to rename this field? > > I didn't grasp from just reading the patches whether this is a uAPI or > just internal kernel flag, seems like the former from the reading of > the comment in patch 2. In which case what guarantees that existing > users don't pass in garbage since the kernel doesn't check it was 0? That's always good to double-check the uapi changes don't break / assume something, thanks for bringing this up. :) Sure, let's go through the possible options step by step. Let me know if I get anything wrong and if I can help with clarifications. There is the "svm_reserved1" field that is not used in the kernel codebase. It is set to 0 on the receive (listen) path as part of the vsock address initialization [1][2]. The "svm_family" and "svm_zero" fields are checked as part of the address validation [3]. Now, with the current change to "svm_flags", the flow is the following: * On the receive (listen) path, the remote address structure is initialized as part of the vsock address init logic [2]. Then patch 3/4 of this series sets the "VMADDR_FLAG_TO_HOST" flag given a set of conditions (local and remote CID > VMADDR_CID_HOST). * On the connect path, the userspace logic can set the "svm_flags" field. It can be set to 0 or 1 (VMADDR_FLAG_TO_HOST); or any other value greater than 1. If the "VMADDR_FLAG_TO_HOST" flag is set, all the vsock packets are then forwarded to the host. * When the vsock transport is assigned, the "svm_flags" field is checked, and if it has the "VMADDR_FLAG_TO_HOST" flag set, it goes on with a guest->host transport (patch 4/4 of this series). Otherwise, other specific flag value is not currently used. Given all these points, the question remains what happens if the "svm_flags" field is set on the connect path to a value higher than 1 (maybe a bogus one, not intended so). And it includes the "VMADDR_FLAG_TO_HOST" value (the single flag set and specifically used for now, but we should also account for any further possible flags). In this case, all the vsock packets would be forwarded to the host and maybe not intended so, having a bogus value for the flags field. Is this possible case what you are referring to? Thanks, Andra [1] https://man7.org/linux/man-pages/man7/vsock.7.html [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/vmw_vsock/vsock_addr.c#n14 [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/vmw_vsock/vsock_addr.c#n23 Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
On Tue, 8 Dec 2020 20:23:24 +0200 Paraschiv, Andra-Irina wrote: > >> --- a/include/uapi/linux/vm_sockets.h > >> +++ b/include/uapi/linux/vm_sockets.h > >> @@ -145,7 +145,7 @@ > >> > >> struct sockaddr_vm { > >> __kernel_sa_family_t svm_family; > >> - unsigned short svm_reserved1; > >> + unsigned short svm_flags; > >> unsigned int svm_port; > >> unsigned int svm_cid; > >> unsigned char svm_zero[sizeof(struct sockaddr) - > > Since this is a uAPI header I gotta ask - are you 100% sure that it's > > okay to rename this field? > > > > I didn't grasp from just reading the patches whether this is a uAPI or > > just internal kernel flag, seems like the former from the reading of > > the comment in patch 2. In which case what guarantees that existing > > users don't pass in garbage since the kernel doesn't check it was 0? > > That's always good to double-check the uapi changes don't break / assume > something, thanks for bringing this up. :) > > Sure, let's go through the possible options step by step. Let me know if > I get anything wrong and if I can help with clarifications. > > There is the "svm_reserved1" field that is not used in the kernel > codebase. It is set to 0 on the receive (listen) path as part of the > vsock address initialization [1][2]. The "svm_family" and "svm_zero" > fields are checked as part of the address validation [3]. > > Now, with the current change to "svm_flags", the flow is the following: > > * On the receive (listen) path, the remote address structure is > initialized as part of the vsock address init logic [2]. Then patch 3/4 > of this series sets the "VMADDR_FLAG_TO_HOST" flag given a set of > conditions (local and remote CID > VMADDR_CID_HOST). > > * On the connect path, the userspace logic can set the "svm_flags" > field. It can be set to 0 or 1 (VMADDR_FLAG_TO_HOST); or any other value > greater than 1. If the "VMADDR_FLAG_TO_HOST" flag is set, all the vsock > packets are then forwarded to the host. > > * When the vsock transport is assigned, the "svm_flags" field is > checked, and if it has the "VMADDR_FLAG_TO_HOST" flag set, it goes on > with a guest->host transport (patch 4/4 of this series). Otherwise, > other specific flag value is not currently used. > > Given all these points, the question remains what happens if the > "svm_flags" field is set on the connect path to a value higher than 1 > (maybe a bogus one, not intended so). And it includes the > "VMADDR_FLAG_TO_HOST" value (the single flag set and specifically used > for now, but we should also account for any further possible flags). In > this case, all the vsock packets would be forwarded to the host and > maybe not intended so, having a bogus value for the flags field. Is this > possible case what you are referring to? Correct. What if user basically declared the structure on the stack, and only initialized the fields the kernel used to check? This problem needs to be at the very least discussed in the commit message.
On Tue, Dec 08, 2020 at 10:42:22AM -0800, Jakub Kicinski wrote: >On Tue, 8 Dec 2020 20:23:24 +0200 Paraschiv, Andra-Irina wrote: >> >> --- a/include/uapi/linux/vm_sockets.h >> >> +++ b/include/uapi/linux/vm_sockets.h >> >> @@ -145,7 +145,7 @@ >> >> >> >> struct sockaddr_vm { >> >> __kernel_sa_family_t svm_family; >> >> - unsigned short svm_reserved1; >> >> + unsigned short svm_flags; >> >> unsigned int svm_port; >> >> unsigned int svm_cid; >> >> unsigned char svm_zero[sizeof(struct sockaddr) - >> > Since this is a uAPI header I gotta ask - are you 100% sure that it's >> > okay to rename this field? >> > >> > I didn't grasp from just reading the patches whether this is a uAPI or >> > just internal kernel flag, seems like the former from the reading of >> > the comment in patch 2. In which case what guarantees that existing >> > users don't pass in garbage since the kernel doesn't check it was 0? >> >> That's always good to double-check the uapi changes don't break / assume >> something, thanks for bringing this up. :) >> >> Sure, let's go through the possible options step by step. Let me know if >> I get anything wrong and if I can help with clarifications. >> >> There is the "svm_reserved1" field that is not used in the kernel >> codebase. It is set to 0 on the receive (listen) path as part of the >> vsock address initialization [1][2]. The "svm_family" and "svm_zero" >> fields are checked as part of the address validation [3]. >> >> Now, with the current change to "svm_flags", the flow is the following: >> >> * On the receive (listen) path, the remote address structure is >> initialized as part of the vsock address init logic [2]. Then patch 3/4 >> of this series sets the "VMADDR_FLAG_TO_HOST" flag given a set of >> conditions (local and remote CID > VMADDR_CID_HOST). >> >> * On the connect path, the userspace logic can set the "svm_flags" >> field. It can be set to 0 or 1 (VMADDR_FLAG_TO_HOST); or any other value >> greater than 1. If the "VMADDR_FLAG_TO_HOST" flag is set, all the vsock >> packets are then forwarded to the host. >> >> * When the vsock transport is assigned, the "svm_flags" field is >> checked, and if it has the "VMADDR_FLAG_TO_HOST" flag set, it goes on >> with a guest->host transport (patch 4/4 of this series). Otherwise, >> other specific flag value is not currently used. >> >> Given all these points, the question remains what happens if the >> "svm_flags" field is set on the connect path to a value higher than 1 >> (maybe a bogus one, not intended so). And it includes the >> "VMADDR_FLAG_TO_HOST" value (the single flag set and specifically used >> for now, but we should also account for any further possible flags). In >> this case, all the vsock packets would be forwarded to the host and >> maybe not intended so, having a bogus value for the flags field. Is this >> possible case what you are referring to? > >Correct. What if user basically declared the structure on the stack, >and only initialized the fields the kernel used to check? > >This problem needs to be at the very least discussed in the commit >message. > I agree that could be a problem, but here some considerations: - I checked some applications (qemu-guest-agent, ncat, iperf-vsock) and all use the same pattern: allocate memory, initialize all the sockaddr_vm to zero (to be sure to initialize the svm_zero), set the cid and port fields. So we should be safe, but of course it may not always be true. - For now the issue could affect only nested VMs. We introduced this support one year ago, so it's something new and maybe we don't cause too many problems. As an alternative, what about using 1 or 2 bytes from svm_zero[]? These must be set at zero, even if we only check the first byte in the kernel. Thanks, Stefano
On 09/12/2020 12:48, Stefano Garzarella wrote: > > On Tue, Dec 08, 2020 at 10:42:22AM -0800, Jakub Kicinski wrote: >> On Tue, 8 Dec 2020 20:23:24 +0200 Paraschiv, Andra-Irina wrote: >>> >> --- a/include/uapi/linux/vm_sockets.h >>> >> +++ b/include/uapi/linux/vm_sockets.h >>> >> @@ -145,7 +145,7 @@ >>> >> >>> >> struct sockaddr_vm { >>> >> __kernel_sa_family_t svm_family; >>> >> - unsigned short svm_reserved1; >>> >> + unsigned short svm_flags; >>> >> unsigned int svm_port; >>> >> unsigned int svm_cid; >>> >> unsigned char svm_zero[sizeof(struct sockaddr) - >>> > Since this is a uAPI header I gotta ask - are you 100% sure that it's >>> > okay to rename this field? >>> > >>> > I didn't grasp from just reading the patches whether this is a >>> uAPI or >>> > just internal kernel flag, seems like the former from the reading of >>> > the comment in patch 2. In which case what guarantees that existing >>> > users don't pass in garbage since the kernel doesn't check it was 0? >>> >>> That's always good to double-check the uapi changes don't break / >>> assume >>> something, thanks for bringing this up. :) >>> >>> Sure, let's go through the possible options step by step. Let me >>> know if >>> I get anything wrong and if I can help with clarifications. >>> >>> There is the "svm_reserved1" field that is not used in the kernel >>> codebase. It is set to 0 on the receive (listen) path as part of the >>> vsock address initialization [1][2]. The "svm_family" and "svm_zero" >>> fields are checked as part of the address validation [3]. >>> >>> Now, with the current change to "svm_flags", the flow is the following: >>> >>> * On the receive (listen) path, the remote address structure is >>> initialized as part of the vsock address init logic [2]. Then patch 3/4 >>> of this series sets the "VMADDR_FLAG_TO_HOST" flag given a set of >>> conditions (local and remote CID > VMADDR_CID_HOST). >>> >>> * On the connect path, the userspace logic can set the "svm_flags" >>> field. It can be set to 0 or 1 (VMADDR_FLAG_TO_HOST); or any other >>> value >>> greater than 1. If the "VMADDR_FLAG_TO_HOST" flag is set, all the vsock >>> packets are then forwarded to the host. >>> >>> * When the vsock transport is assigned, the "svm_flags" field is >>> checked, and if it has the "VMADDR_FLAG_TO_HOST" flag set, it goes on >>> with a guest->host transport (patch 4/4 of this series). Otherwise, >>> other specific flag value is not currently used. >>> >>> Given all these points, the question remains what happens if the >>> "svm_flags" field is set on the connect path to a value higher than 1 >>> (maybe a bogus one, not intended so). And it includes the >>> "VMADDR_FLAG_TO_HOST" value (the single flag set and specifically used >>> for now, but we should also account for any further possible flags). In >>> this case, all the vsock packets would be forwarded to the host and >>> maybe not intended so, having a bogus value for the flags field. Is >>> this >>> possible case what you are referring to? >> >> Correct. What if user basically declared the structure on the stack, >> and only initialized the fields the kernel used to check? >> >> This problem needs to be at the very least discussed in the commit >> message. >> > > I agree that could be a problem, but here some considerations: > - I checked some applications (qemu-guest-agent, ncat, iperf-vsock) and > all use the same pattern: allocate memory, initialize all the > sockaddr_vm to zero (to be sure to initialize the svm_zero), set the > cid and port fields. > So we should be safe, but of course it may not always be true. > > - For now the issue could affect only nested VMs. We introduced this > support one year ago, so it's something new and maybe we don't cause > too many problems. > > As an alternative, what about using 1 or 2 bytes from svm_zero[]? > These must be set at zero, even if we only check the first byte in the > kernel. Thanks for the follow-up info. We can also consider the "svm_zero" option and could use 2 bytes from that field for "svm_flags", keeping the same "unsigned short" type. Thanks, Andra Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
On Wed, 9 Dec 2020 17:17:56 +0200 Paraschiv, Andra-Irina wrote: > > I agree that could be a problem, but here some considerations: > > - I checked some applications (qemu-guest-agent, ncat, iperf-vsock) and > > all use the same pattern: allocate memory, initialize all the > > sockaddr_vm to zero (to be sure to initialize the svm_zero), set the > > cid and port fields. > > So we should be safe, but of course it may not always be true. > > > > - For now the issue could affect only nested VMs. We introduced this > > support one year ago, so it's something new and maybe we don't cause > > too many problems. > > > > As an alternative, what about using 1 or 2 bytes from svm_zero[]? > > These must be set at zero, even if we only check the first byte in the > > kernel. > > Thanks for the follow-up info. > > We can also consider the "svm_zero" option and could use 2 bytes from > that field for "svm_flags", keeping the same "unsigned short" type. Or use svm_zero as a gate for interpreting other fields? If svm_zero[0]* == something start checking the value of reserved1? * in practice the name can be unioned to something more palatable ;)
On 09/12/2020 19:30, Jakub Kicinski wrote: > On Wed, 9 Dec 2020 17:17:56 +0200 Paraschiv, Andra-Irina wrote: >>> I agree that could be a problem, but here some considerations: >>> - I checked some applications (qemu-guest-agent, ncat, iperf-vsock) and >>> all use the same pattern: allocate memory, initialize all the >>> sockaddr_vm to zero (to be sure to initialize the svm_zero), set the >>> cid and port fields. >>> So we should be safe, but of course it may not always be true. >>> >>> - For now the issue could affect only nested VMs. We introduced this >>> support one year ago, so it's something new and maybe we don't cause >>> too many problems. >>> >>> As an alternative, what about using 1 or 2 bytes from svm_zero[]? >>> These must be set at zero, even if we only check the first byte in the >>> kernel. >> Thanks for the follow-up info. >> >> We can also consider the "svm_zero" option and could use 2 bytes from >> that field for "svm_flags", keeping the same "unsigned short" type. > Or use svm_zero as a gate for interpreting other fields? > If svm_zero[0]* == something start checking the value of reserved1? > * in practice the name can be unioned to something more palatable ;) Thanks for the shared option, that could be one case to reuse the reserved field, with a two phase check logic. I'll give it a try to the option of having a new field "svm_flags" and the "svm_zero" updated and then send out a new revision. Just let me know if there are other updates needed / questions in the meantime. struct sockaddr_vm { __kernel_sa_family_t svm_family; unsigned short svm_reserved1; unsigned int svm_port; unsigned int svm_cid; unsigned short svm_flags; unsigned char svm_zero[sizeof(struct sockaddr) - sizeof(sa_family_t) - sizeof(unsigned short) - sizeof(unsigned int) - sizeof(unsigned int) - sizeof(unsigned short)]; }; Thanks, Andra Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h index fd0ed7221645d..46735376a57a8 100644 --- a/include/uapi/linux/vm_sockets.h +++ b/include/uapi/linux/vm_sockets.h @@ -145,7 +145,7 @@ struct sockaddr_vm { __kernel_sa_family_t svm_family; - unsigned short svm_reserved1; + unsigned short svm_flags; unsigned int svm_port; unsigned int svm_cid; unsigned char svm_zero[sizeof(struct sockaddr) -
vsock enables communication between virtual machines and the host they are running on. With the multi transport support (guest->host and host->guest), nested VMs can also use vsock channels for communication. In addition to this, by default, all the vsock packets are forwarded to the host, if no host->guest transport is loaded. This behavior can be implicitly used for enabling vsock communication between sibling VMs. Add a flags field in the vsock address data structure that can be used to explicitly mark the vsock connection as being targeted for a certain type of communication. This way, can distinguish between different use cases such as nested VMs and sibling VMs. Use the already available "svm_reserved1" field and mark it as a flags field instead. This field can be set when initializing the vsock address variable used for the connect() call. Changelog v1 -> v2 * Update the field name to "svm_flags". * Split the current patch in 2 patches. Signed-off-by: Andra Paraschiv <andraprs@amazon.com> --- include/uapi/linux/vm_sockets.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)