GitLab
gitlab-to-mail https://gitlab.winehq.org/project_2_bot started a new discussion https://gitlab.winehq.org/wine/wine/-/merge_requests/384#note_4039:
Zebediah Figura replied on the mailing list:
|On 7/5/22 14:53, Paul Gofman wrote: @@ -602,9 +623,68 @@ static NTSTATUS try_recv( int fd, struct async_recv_ioctl *async, ULONG_PTR *siz } status = (hdr.msg_flags & MSG_TRUNC) ? STATUS_BUFFER_OVERFLOW : STATUS_SUCCESS; + if (async->fixup_type == SOCK_PROT_FIXUP_ICMP_OVER_DGRAM) + { + unsigned int tot_len = sizeof(struct ip_hdr) + ret; + size_t len = hdr.msg_iov[0].iov_len; + char *buf = hdr.msg_iov[0].iov_base; + struct cmsghdr *cmsg; + struct ip_hdr ip_h; + + if (ret + sizeof(ip_h) > len) + status = STATUS_BUFFER_OVERFLOW; + + if (len < sizeof(ip_h)) + { + ret = len; + } + else + { + ret = min( ret, len - sizeof(ip_h) ); + memmove( buf + sizeof(ip_h), buf, ret ); + ret += sizeof(ip_h); + } + memset( &ip_h, 0, sizeof(ip_h) ); + ip_h.v_hl = (4 << 4) | (sizeof(ip_h) >> 2); + ip_h.tot_len = htons( tot_len ); + ip_h.protocol = 1; + ip_h.saddr = unix_addr.in.sin_addr.s_addr; + + for (cmsg = CMSG_FIRSTHDR( &hdr ); cmsg; cmsg = CMSG_NXTHDR( &hdr, cmsg )) + { + if (cmsg->cmsg_level != IPPROTO_IP) continue; + switch (cmsg->cmsg_type) + { +#if defined(IP_TTL) + case IP_TTL: + ip_h.ttl = *(BYTE *)CMSG_DATA( cmsg ); + break; +#endif +#if defined(IP_TOS) + case IP_TOS: + ip_h.tos = *(BYTE *)CMSG_DATA( cmsg ); + break; +#endif +#if defined(IP_PKTINFO)
- case IP_PKTINFO: + { + struct in_pktinfo *info; + + info = (struct
in_pktinfo *)CMSG_DATA( cmsg ); + ip_h.daddr = info->ipi_addr.s_addr;
- break; + } +#endif + } + } + memcpy( buf, &ip_h, min( sizeof(ip_h),
len )); + } Would you mind making this a helper function? try_recv() is already large and this seems to be pretty self-contained logic.|
|Sure.|
|> if (async->control) > { > + if (async->fixup_type == SOCK_PROT_FIXUP_ICMP_OVER_DGRAM) > + FIXME( "May return extra control headers.\n" ); > + What is this for?|
When initializing the fixup on server I am setting IP_RECVTTL, IP_RECVTOS and IP_PKTINFO socket options. If the app is calling recvmsg() it may get the headers it didn't request. My reasoning that such a situation doesn't deserve further code complication to avoid but deserves a fixme.
|> @@ -737,17 +818,26 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi > } > } > > - SERVER_START_REQ( recv_socket ) > + do > { > - req->force_async = force_async; > - req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) ); > - req->oob = !!(unix_flags & MSG_OOB); > - status = wine_server_call( req ); > - wait_handle = wine_server_ptr_handle( reply->wait ); > - options = reply->options; >
- nonblocking = reply->nonblocking; > - } > - SERVER_END_REQ; > +
SERVER_START_REQ( recv_socket ) > + { > + req->force_async = force_async; > + req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) ); > + req->oob = !!(unix_flags & MSG_OOB); > + req->fixup_type = async->fixup_type; > + status = wine_server_call( req ); > + if (status == STATUS_MORE_PROCESSING_REQUIRED) > + { > + async->fixup_type = reply->fixup_type; > + continue; > + } > + wait_handle = wine_server_ptr_handle( reply->wait ); > + options = reply->options; >
- nonblocking = reply->nonblocking; > + } > + SERVER_END_REQ; > + }
while (status == STATUS_MORE_PROCESSING_REQUIRED); > > alerted = status == STATUS_ALERTED; > if (alerted) I'm missing something; why do we need to call this in a loop? Why does the client needs to communicate the fixup type to the server in the first place?|
The async may be initiated on the server and delivered to the process (through SIGUSR1)�� before we set async->fixup_type. It might be not likely in practice, but I double verified that it is possible by injecting Sleep(...) before setting fixup type to async structure from server. So I need to make sure that fixup_type in async structure is correctly set before try_recv() can be reached. So idea is, fixup_type is "no fixup" by default, no extra server call is involved in normal case. But if fixup is required and client does not yet know about it server doesn't�� do anything and just returns fixup type with STATUS_MORE_PROCESSING_REQUIRED.
|> diff --git a/server/protocol.def b/server/protocol.def > index a8beccaf468..e1e5f3d9faa 100644 > --- a/server/protocol.def > +++ b/server/protocol.def > @@ -1446,17 +1446,23 @@ enum server_fd_type > file_pos_t count; /* count of bytes to unlock */ > @END > > - > /* Perform a recv on a socket */ > @REQ(recv_socket) > int oob; /* are we receiving OOB data? */ > async_data_t async; /* async I/O parameters */ > int force_async; /* Force asynchronous mode? */ > + int fixup_type; /* protocol fixup type returned previously */ > @REPLY > obj_handle_t wait; /* handle to wait on for blocking recv */ > unsigned int options; /* device open options */ > int nonblocking; /* is socket non-blocking? */ > + int fixup_type; /* protocol fixup type (see below) */ > @END > +enum sock_prot_fixup_type > +{ > + SOCK_PROT_FIXUP_NONE, > + SOCK_PROT_FIXUP_ICMP_OVER_DGRAM, > +}; Making this an enum strikes me as odd in general. A bitmask might make more sense, but frankly I'd just leave it as a dedicated field until we have something else that needs fixups. Do you have any plans for introducing other types of fixups?|
Well, originally I did something between the lines of this (had icmp_over_dgram field in request / reply). But then I looked at that and it felt odd that general recv_socket() has some special icmp related fields, it felt like naming that "fixup_type" tells what it is without going into very specific ICMP fixup details looking weird at this level. If you feel like it is better to name it, e. g., "icmp_over_dgram" I can change it back. I don't currently have any other types of fixups in mind.
On 7/12/22 22:29, Paul Gofman (@gofman) wrote:
When initializing the fixup on server I am setting IP_RECVTTL, IP_RECVTOS and IP_PKTINFO socket options. If the app is calling recvmsg() it may get the headers it didn't request. My reasoning that such a situation doesn't deserve further code complication to avoid but deserves a fixme.
Makes sense, thanks.
The async may be initiated on the server and delivered to the process (through SIGUSR1)?? before we set async->fixup_type. It might be not likely in practice, but I double verified that it is possible by injecting Sleep(...) before setting fixup type to async structure from server. So I need to make sure that fixup_type in async structure is correctly set before try_recv() can be reached. So idea is, fixup_type is "no fixup" by default, no extra server call is involved in normal case. But if fixup is required and client does not yet know about it server doesn't?? do anything and just returns fixup type with STATUS_MORE_PROCESSING_REQUIRED.
Right, that is annoying, but I guess it's the most reasonable way to solve that problem.
On the other hand... spitballing, but what if instead of returning anything from the server, we used SO_PROTOCOL and SO_TYPE on the client side to check whether it's a DGRAM + ICMP socket?
Making this an enum strikes me as odd in general. A bitmask might make more sense, but frankly I'd just leave it as a dedicated field until we have something else that needs fixups. Do you have any plans for introducing other types of fixups?|
Well, originally I did something between the lines of this (had icmp_over_dgram field in request / reply). But then I looked at that and it felt odd that general recv_socket() has some special icmp related fields, it felt like naming that "fixup_type" tells what it is without going into very specific ICMP fixup details looking weird at this level. If you feel like it is better to name it, e. g., "icmp_over_dgram" I can change it back. I don't currently have any other types of fixups in mind.
On reflection I don't feel strongly about it. I think "is_icmp_dgram" or something is reasonable enough by itself if clarified with a comment, but I can also understand the opposing argument.
On 7/14/22 13:51, Zebediah Figura wrote:
The async may be initiated on the server and delivered to the process (through SIGUSR1)?? before we set async->fixup_type. It might be not likely in practice, but I double verified that it is possible by injecting Sleep(...) before setting fixup type to async structure from server. So I need to make sure that fixup_type in async structure is correctly set before try_recv() can be reached. So idea is, fixup_type is "no fixup" by default, no extra server call is involved in normal case. But if fixup is required and client does not yet know about it server doesn't?? do anything and just returns fixup type with STATUS_MORE_PROCESSING_REQUIRED.
Right, that is annoying, but I guess it's the most reasonable way to solve that problem.
On the other hand... spitballing, but what if instead of returning anything from the server, we used SO_PROTOCOL and SO_TYPE on the client side to check whether it's a DGRAM + ICMP socket?
I am not sure how do I get that info on the client now without asking server? I could do native getsockopt() ofc but IMO it would be not desirable to have extra calls for just every socket operation motivated by this special case. Currently the overhead is contained for this very specific case only.
Making this an enum strikes me as odd in general. A bitmask might make more sense, but frankly I'd just leave it as a dedicated field until we have something else that needs fixups. Do you have any plans for introducing other types of fixups?|
Well, originally I did something between the lines of this (had icmp_over_dgram field in request / reply). But then I looked at that and it felt odd that general recv_socket() has some special icmp related fields, it felt like naming that "fixup_type" tells what it is without going into very specific ICMP fixup details looking weird at this level. If you feel like it is better to name it, e. g., "icmp_over_dgram" I can change it back. I don't currently have any other types of fixups in mind.
On reflection I don't feel strongly about it. I think "is_icmp_dgram" or something is reasonable enough by itself if clarified with a comment, but I can also understand the opposing argument.
Well, I started changing it back already so lets do it this way, I guess.
wine-gitlab mailing list -- wine-gitlab@winehq.org To unsubscribe send an email to wine-gitlab-leave@winehq.org
On 7/14/22 13:59, Paul Gofman wrote:
On 7/14/22 13:51, Zebediah Figura wrote:
The async may be initiated on the server and delivered to the process (through SIGUSR1)?? before we set async->fixup_type. It might be not likely in practice, but I double verified that it is possible by injecting Sleep(...) before setting fixup type to async structure from server. So I need to make sure that fixup_type in async structure is correctly set before try_recv() can be reached. So idea is, fixup_type is "no fixup" by default, no extra server call is involved in normal case. But if fixup is required and client does not yet know about it server doesn't?? do anything and just returns fixup type with STATUS_MORE_PROCESSING_REQUIRED.
Right, that is annoying, but I guess it's the most reasonable way to solve that problem.
On the other hand... spitballing, but what if instead of returning anything from the server, we used SO_PROTOCOL and SO_TYPE on the client side to check whether it's a DGRAM + ICMP socket?
I am not sure how do I get that info on the client now without asking server? I could do native getsockopt() ofc but IMO it would be not desirable to have extra calls for just every socket operation motivated by this special case. Currently the overhead is contained for this very specific case only.
Yes, getsockopt() on the client side.
Although it means extra syscalls, these should be fast syscalls (they are just retrieving static data from the socket), and socket code is not really one where syscalls themselves are the bottleneck. We're making at least two server calls for every send/recv operation, not counting the actual send()/recv() call. If syscalls are going to be a problem I suspect there are other avenues we'll need to pursue first.
On 7/14/22 14:03, Zebediah Figura wrote:
Yes, getsockopt() on the client side.
Although it means extra syscalls, these should be fast syscalls (they are just retrieving static data from the socket), and socket code is not really one where syscalls themselves are the bottleneck. We're making at least two server calls for every send/recv operation, not counting the actual send()/recv() call. If syscalls are going to be a problem I suspect there are other avenues we'll need to pursue first.
Makes sense, yeah, an extra native syscall might not be an actual issue in this case. But if I understand correctly ICMP on a datagram socket should work on Windows as well (at least in some way) and in this case it should not be fixed up. I am not sure that winsock ICMP over dgram will currently work correctly in Wine anyway, but not sure making it worse by unrelated fixing up makes sense. I don't seem to be finding any socket option allowing to attach user-defined info to the socket. We could in theory abuse some but that looks weird. Can you think of some way to do it without making it uglier (like abusing some totally unrelated socket option)?
On 7/14/22 14:29, Paul Gofman wrote:
On 7/14/22 14:03, Zebediah Figura wrote:
Yes, getsockopt() on the client side.
Although it means extra syscalls, these should be fast syscalls (they are just retrieving static data from the socket), and socket code is not really one where syscalls themselves are the bottleneck. We're making at least two server calls for every send/recv operation, not counting the actual send()/recv() call. If syscalls are going to be a problem I suspect there are other avenues we'll need to pursue first.
Makes sense, yeah, an extra native syscall might not be an actual issue in this case. But if I understand correctly ICMP on a datagram socket should work on Windows as well (at least in some way) and in this case it should not be fixed up. I am not sure that winsock ICMP over dgram will currently work correctly in Wine anyway, but not sure making it worse by unrelated fixing up makes sense. I don't seem to be finding any socket option allowing to attach user-defined info to the socket. We could in theory abuse some but that looks weird. Can you think of some way to do it without making it uglier (like abusing some totally unrelated socket option)?
It doesn't seem to be supported for me; creating a socket with IPPROTO_ICMP and SOCK_DGRAM on Windows fails with WSAENOPROTOOPT.
On 7/14/22 14:43, Zebediah Figura wrote:
On 7/14/22 14:29, Paul Gofman wrote:
On 7/14/22 14:03, Zebediah Figura wrote:
Yes, getsockopt() on the client side.
Although it means extra syscalls, these should be fast syscalls (they are just retrieving static data from the socket), and socket code is not really one where syscalls themselves are the bottleneck. We're making at least two server calls for every send/recv operation, not counting the actual send()/recv() call. If syscalls are going to be a problem I suspect there are other avenues we'll need to pursue first.
Makes sense, yeah, an extra native syscall might not be an actual issue in this case. But if I understand correctly ICMP on a datagram socket should work on Windows as well (at least in some way) and in this case it should not be fixed up. I am not sure that winsock ICMP over dgram will currently work correctly in Wine anyway, but not sure making it worse by unrelated fixing up makes sense. I don't seem to be finding any socket option allowing to attach user-defined info to the socket. We could in theory abuse some but that looks weird. Can you think of some way to do it without making it uglier (like abusing some totally unrelated socket option)?
It doesn't seem to be supported for me; creating a socket with IPPROTO_ICMP and SOCK_DGRAM on Windows fails with WSAENOPROTOOPT.
Well, sorry, I must be wrong about that then. So let's go this way, I will update the patches. That will remove the additional fields in socket receive / send server calls and handling of those, just the new directly fixup-related calls will stay.
-
Paul Gofman -
Paul Gofman (＠gofman) -
Zebediah Figura