[PATCH] device: revert pipelining UAPI requests

[PATCH] device: revert pipelining UAPI requests

[Laura Zelenku]

Hello devs,
there is an issue when using wg-quick tool and wg tool bellow from wireguard-tools repository (version v1.0.20210223). I’m running: wg-quick up operation with config file in /etc/wireguard/ directory. Get operations are ok. Processed without a problem. But for Set operation the processing will stuck on "buffered.ReadString('\n’)” (in the second run of for loop cycle, after set command is processed in first for loop cycle) because bufio.Scanner will read everything from the socket.
Please revert till proper fix is ready.

thanks
Laura

Signed-off-by: Laura Zelenku <laura.zelenku@wandera.com>
---
 device/uapi.go | 64 +++++++++++++++++++++-----------------------------
 1 file changed, 27 insertions(+), 37 deletions(-)

diff --git a/device/uapi.go b/device/uapi.go
index 659af0a..9fab911 100644
--- a/device/uapi.go
+++ b/device/uapi.go
@@ -414,44 +414,34 @@ func (device *Device) IpcHandle(socket net.Conn) {
                return bufio.NewReadWriter(reader, writer)
        }(socket)
 
-       for {
-               op, err := buffered.ReadString('\n')
-               if err != nil {
-                       return
-               }
+       defer buffered.Flush()
 
-               // handle operation
-               switch op {
-               case "set=1\n":
-                       err = device.IpcSetOperation(buffered.Reader)
-               case "get=1\n":
-                       var nextByte byte
-                       nextByte, err = buffered.ReadByte()
-                       if err != nil {
-                               return
-                       }
-                       if nextByte != '\n' {
-                               err = ipcErrorf(ipc.IpcErrorInvalid, "trailing character in UAPI get: %q", nextByte)
-                               break
-                       }
-                       err = device.IpcGetOperation(buffered.Writer)
-               default:
-                       device.log.Errorf("invalid UAPI operation: %v", op)
-                       return
-               }
+       op, err := buffered.ReadString('\n')
+       if err != nil {
+               return
+       }
 
-               // write status
-               var status *IPCError
-               if err != nil && !errors.As(err, &status) {
-                       // shouldn't happen
-                       status = ipcErrorf(ipc.IpcErrorUnknown, "other UAPI error: %w", err)
-               }
-               if status != nil {
-                       device.log.Errorf("%v", status)
-                       fmt.Fprintf(buffered, "errno=%d\n\n", status.ErrorCode())
-               } else {
-                       fmt.Fprintf(buffered, "errno=0\n\n")
-               }
-               buffered.Flush()
+       // handle operation
+       switch op {
+       case "set=1\n":
+               err = device.IpcSetOperation(buffered.Reader)
+       case "get=1\n":
+               err = device.IpcGetOperation(buffered.Writer)
+       default:
+               device.log.Errorf("invalid UAPI operation: %v", op)
+               return
+       }
+
+       // write status
+       var status *IPCError
+       if err != nil && !errors.As(err, &status) {
+               // shouldn't happen
+               status = ipcErrorf(ipc.IpcErrorUnknown, "other UAPI error: %w", err)
+       }
+       if status != nil {
+               device.log.Errorf("%v", status)
+               fmt.Fprintf(buffered, "errno=%d\n\n", status.ErrorCode())
+       } else {
+               fmt.Fprintf(buffered, "errno=0\n\n")
        }
 }
-- 
2.28.0





-- 
*IMPORTANT NOTICE*: This email, its attachments and any rights attaching 
hereto are confidential and intended exclusively for the person to whom the 
email is addressed. If you are not the intended recipient, do not read, 
copy, disclose or use the contents in any way. Wandera accepts no liability 
for any loss, damage or consequence resulting directly or indirectly from 
the use of this email and attachments.

Re: [PATCH] device: revert pipelining UAPI requests

[Jason A. Donenfeld]

On Tue, Mar 9, 2021 at 8:04 AM Laura Zelenku <laura.zelenku@wandera.com> wrote:
>
> Hello devs,
> there is an issue when using wg-quick tool and wg tool bellow from wireguard-tools repository (version v1.0.20210223). I’m running: wg-quick up operation with config file in /etc/wireguard/ directory. Get operations are ok. Processed without a problem. But for Set operation the processing will stuck on "buffered.ReadString('\n’)” (in the second run of for loop cycle, after set command is processed in first for loop cycle) because bufio.Scanner will read everything from the socket.

I'm unable to reproduce this issue. Can you send a small shell script
that does so?

> Please revert till proper fix is ready.

How hard can a proper fix be? Can't we just commit the actual fix instead?

Jason

Re: [PATCH] device: revert pipelining UAPI requests

[Laura Zelenku]

Hi Jason,
I’m just using “wg-quick up wg0” command on Linux env with NO kernel module, just userspace implementation.
My config file looks like:
[Interface]
PrivateKey = ….mEw=
Address = fddd:dddd:1000::1/128
DNS = fddd:dddd::
MTU = 1420
[Peer]
PublicKey = …..pC0=
AllowedIPs = fd53::/32,fddd:dddd::/128
Endpoint = dev.wandera.com:3005
PersistentKeepalive = 25

In the command output you can see that "wg setconf wg0 /dev/fd/63” stucked and no following commands (set IP address, set MTU, …) are processed.
The output is:

# wg-quick up wg0
[#] ip link add wg0 type wireguard
RTNETLINK answers: Not supported
[!] Missing WireGuard kernel module. Falling back to slow userspace implementation.
[#] /wireguard/wireguard-go wg0
┌───────────────────────────────────────────────────┐
│                                                   │
│   Running this software on Linux is unnecessary,  │
│   because the Linux kernel has built-in first     │
│   class support for WireGuard, which will be      │
│   faster, slicker, and better integrated. For     │
│   information on installing the kernel module,    │
│   please visit: <https://wireguard.com/install>.  │
│                                                   │
└───────────────────────────────────────────────────┘
DEBUG: (wg0) 2021/03/09 13:11:41 Starting wireguard-go version 0.0.20210212-dirty
DEBUG: (wg0) 2021/03/09 13:11:41 Starting wireguard-go version 0.0.20210212-dirty
DEBUG: (wg0) 2021/03/09 13:11:41 Device started
DEBUG: (wg0) 2021/03/09 13:11:41 UAPI listener started
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: decryption worker - started
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: encryption worker - started
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: decryption worker - started
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: handshake worker - started
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: encryption worker - started
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: handshake worker - started
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: handshake worker - started
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: encryption worker - started
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: decryption worker - started
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: decryption worker - started
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: encryption worker - started
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: handshake worker - started
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: TUN reader - started
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: event worker - started
DEBUG: (wg0) 2021/03/09 13:11:41 Interface up requested
DEBUG: (wg0) 2021/03/09 13:11:41 UDP bind has been updated
DEBUG: (wg0) 2021/03/09 13:11:41 Interface state was Down, requested Up, now Up
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: receive incoming IPv6 - started
[#] wg setconf wg0 /dev/fd/63
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: receive incoming IPv4 - started
DEBUG: (wg0) 2021/03/09 13:11:41 UAPI: Updating private key
DEBUG: (wg0) 2021/03/09 13:11:41 UAPI: Updating listen port
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: receive incoming IPv4 - stopped
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: receive incoming IPv6 - stopped
DEBUG: (wg0) 2021/03/09 13:11:41 UDP bind has been updated
DEBUG: (wg0) 2021/03/09 13:11:41 UAPI: Updating fwmark
DEBUG: (wg0) 2021/03/09 13:11:41 UAPI: Removing all peers
DEBUG: (wg0) 2021/03/09 13:11:41 peer(0DIu…qpC0) - Starting...
DEBUG: (wg0) 2021/03/09 13:11:41 peer(0DIu…qpC0) - UAPI: Created
DEBUG: (wg0) 2021/03/09 13:11:41 peer(0DIu…qpC0) - UAPI: Updating endpoint
DEBUG: (wg0) 2021/03/09 13:11:41 peer(0DIu…qpC0) - UAPI: Updating persistent keepalive interval
DEBUG: (wg0) 2021/03/09 13:11:41 peer(0DIu…qpC0) - Sending keepalive packet
DEBUG: (wg0) 2021/03/09 13:11:41 peer(0DIu…qpC0) - Sending handshake initiation
DEBUG: (wg0) 2021/03/09 13:11:41 peer(0DIu…qpC0) - UAPI: Removing all allowedips
DEBUG: (wg0) 2021/03/09 13:11:41 peer(0DIu…qpC0) - UAPI: Adding allowedip
DEBUG: (wg0) 2021/03/09 13:11:41 peer(0DIu…qpC0) - UAPI: Adding allowedip
DEBUG: (wg0) 2021/03/09 13:11:41 peer(0DIu…qpC0) - Routine: sequential receiver - started
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: receive incoming IPv4 - started
DEBUG: (wg0) 2021/03/09 13:11:41 Routine: receive incoming IPv6 - started
DEBUG: (wg0) 2021/03/09 13:11:41 peer(0DIu…qpC0) - Routine: sequential sender - started
DEBUG: (wg0) 2021/03/09 13:11:41 peer(0DIu…qpC0) - Received handshake response
DEBUG: (wg0) 2021/03/09 13:11:42 Interface up requested
DEBUG: (wg0) 2021/03/09 13:11:43 Interface up requested
DEBUG: (wg0) 2021/03/09 13:11:44 Interface up requested
DEBUG: (wg0) 2021/03/09 13:11:45 Interface up requested

As I’m thinking about proper fix I have no idea. The solution would also be to “return” after first set command (no second loop processing).

Laura


> On 9 Mar 2021, at 16:09, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> 
> On Tue, Mar 9, 2021 at 8:04 AM Laura Zelenku <laura.zelenku@wandera.com> wrote:
>> 
>> Hello devs,
>> there is an issue when using wg-quick tool and wg tool bellow from wireguard-tools repository (version v1.0.20210223). I’m running: wg-quick up operation with config file in /etc/wireguard/ directory. Get operations are ok. Processed without a problem. But for Set operation the processing will stuck on "buffered.ReadString('\n’)” (in the second run of for loop cycle, after set command is processed in first for loop cycle) because bufio.Scanner will read everything from the socket.
> 
> I'm unable to reproduce this issue. Can you send a small shell script
> that does so?
> 
>> Please revert till proper fix is ready.
> 
> How hard can a proper fix be? Can't we just commit the actual fix instead?
> 
> Jason


-- 
*IMPORTANT NOTICE*: This email, its attachments and any rights attaching 
hereto are confidential and intended exclusively for the person to whom the 
email is addressed. If you are not the intended recipient, do not read, 
copy, disclose or use the contents in any way. Wandera accepts no liability 
for any loss, damage or consequence resulting directly or indirectly from 
the use of this email and attachments.

Re: [PATCH] device: revert pipelining UAPI requests

[Jason A. Donenfeld]

On Tue, Mar 9, 2021 at 9:30 AM Laura Zelenku <laura.zelenku@wandera.com> wrote:
>
> Hi Jason,
> I’m just using “wg-quick up wg0” command on Linux env with NO kernel module, just userspace implementation.
> My config file looks like:
> [Interface]
> PrivateKey = ….mEw=

Change your private key. No need to mess with partial exposures.

> In the command output you can see that "wg setconf wg0 /dev/fd/63” stucked and no following commands (set IP address, set MTU, …) are processed.

What's the output of `wg --version`?

Jason

Re: [PATCH] device: revert pipelining UAPI requests

[Laura Zelenku]

Hello Jason,
I’ve found there was version wireguard-tools v1.0.20200510. Version v1.0.20210223 builded from the source was overriden by package installation in the container :-(
I’ve alligned versions (wireguard-tools v1.0.20210223) and it is working as expected.

I’m sorry for false alert and thank you for your assistance.
Laura

> On 9 Mar 2021, at 17:33, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> 
> On Tue, Mar 9, 2021 at 9:30 AM Laura Zelenku <laura.zelenku@wandera.com> wrote:
>> 
>> Hi Jason,
>> I’m just using “wg-quick up wg0” command on Linux env with NO kernel module, just userspace implementation.
>> My config file looks like:
>> [Interface]
>> PrivateKey = ….mEw=
> 
> Change your private key. No need to mess with partial exposures.
> 
>> In the command output you can see that "wg setconf wg0 /dev/fd/63” stucked and no following commands (set IP address, set MTU, …) are processed.
> 
> What's the output of `wg --version`?
> 
> Jason


-- 
*IMPORTANT NOTICE*: This email, its attachments and any rights attaching 
hereto are confidential and intended exclusively for the person to whom the 
email is addressed. If you are not the intended recipient, do not read, 
copy, disclose or use the contents in any way. Wandera accepts no liability 
for any loss, damage or consequence resulting directly or indirectly from 
the use of this email and attachments.