Re: [Libguestfs] [nbdkit PATCH] nbd: Opt in to libnbd pread_initialize speedup
On 02/10/22 16:38, Eric Blake wrote:
> Our nbd plugin has always properly checked for asynch errors (and thus
> has never been at risk of a vulnerability similar to CVE-2022-0485
> just fixed in nbdcopy). What's more, the nbdkit core guarantees
> (since commit b1ce255e in 2019, v1.13.1) that the buffer handed to a
> .pread callback has been pre-sanitized to not leak heap contents,
> regardless of how buggy a plugin might be (see
> server/protocol.c:protocol_recv_request_send_reply(), which uses
> server/threadlocal.c:threadlocal_buffer() for a safe buffer). Thus,
> we do not need libnbd to spend time re-initializing the buffer, if
> libnbd is new enough to give us that knob.
> ---
>
> I know that Laszlo was sceptical whether any real client might want to
> use the libnbd knob [1], but I'm actually comfortable with this one.
>
> [1] https://listman.redhat.com/archives/libguestfs/2022-February/msg00144.html
>
> plugins/nbd/nbd.c | 11 ++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
> index ae595ea7..2e154e8d 100644
> --- a/plugins/nbd/nbd.c
> +++ b/plugins/nbd/nbd.c
> @@ -1,5 +1,5 @@
> /* nbdkit
> - * Copyright (C) 2017-2020, 2022 Red Hat Inc.
> + * Copyright (C) 2017-2022 Red Hat Inc.
> *
> * Redistribution and use in source and binary forms, with or without
> * modification, are permitted provided that the following conditions are
> @@ -637,6 +637,15 @@ nbdplug_open_handle (int readonly, const char
> *client_export)
> #if LIBNBD_HAVE_NBD_SET_FULL_INFO
>if (nbd_set_full_info (h->nbd, 1) == -1)
> goto errnbd;
> +#endif
> +#if LIBNBD_HAVE_NBD_SET_PREAD_INITIALIZE
> + /* nbdkit guarantees that the buffers passed to our .pread callback
> + * are pre-initialized; and we in turn ensure that the buffer is not
> + * dereferenced if the NBD server replied with an error. Thus, we
> + * are safe opting in to this libnbd speedup.
> + */
> + if (nbd_set_pread_initialize (h->nbd, false) == -1)
> +goto errnbd;
> #endif
>if (dynamic_export && uri) {
> #if LIBNBD_HAVE_NBD_SET_OPT_MODE
>
hahaha, this was real quick ;)
Acked-by: Laszlo Ersek
Laszlo
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH libnbd v2 6/9] golang: tests: Use AioBuffer.Slice()
Slice() is easier to use and faster than Get() or Bytes(). Let's use the
new way.
Signed-off-by: Nir Soffer
---
golang/libnbd_020_aio_buffer_test.go | 8 +---
golang/libnbd_500_aio_pread_test.go | 2 +-
golang/libnbd_510_aio_pwrite_test.go | 8 +---
3 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/golang/libnbd_020_aio_buffer_test.go
b/golang/libnbd_020_aio_buffer_test.go
index 4b1c5f93..e07f8973 100644
--- a/golang/libnbd_020_aio_buffer_test.go
+++ b/golang/libnbd_020_aio_buffer_test.go
@@ -22,22 +22,23 @@ import (
"bytes"
"testing"
)
func TestAioBuffer(t *testing.T) {
/* Create a buffer with uninitialized backing array. */
buf := MakeAioBuffer(uint(32))
defer buf.Free()
/* Initialize backing array contents. */
+ s := buf.Slice()
for i := uint(0); i < buf.Size; i++ {
- *buf.Get(i) = 0
+ s[i] = 0
}
/* Create a slice by copying the backing array contents into Go memory.
*/
b := buf.Bytes()
zeroes := make([]byte, 32)
if !bytes.Equal(b, zeroes) {
t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes())
}
@@ -45,21 +46,21 @@ func TestAioBuffer(t *testing.T) {
for i := 0; i < len(b); i++ {
b[i] = 42
}
/* Bytes() still returns zeroes. */
if !bytes.Equal(buf.Bytes(), zeroes) {
t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes())
}
/* Creating a slice without copying the underlying buffer. */
- s := buf.Slice()
+ s = buf.Slice()
if !bytes.Equal(s, zeroes) {
t.Fatalf("Expected %v, got %v", zeroes, s)
}
/* Modifying the slice modifies the underlying buffer. */
for i := 0; i < len(s); i++ {
s[i] = 42
}
if !bytes.Equal(buf.Slice(), s) {
@@ -154,22 +155,23 @@ func BenchmarkMakeAioBufferZero(b *testing.B) {
for i := 0; i < b.N; i++ {
buf := MakeAioBufferZero(bufferSize)
buf.Free()
}
}
// Benchmark zeroing a buffer.
func BenchmarkAioBufferZero(b *testing.B) {
for i := 0; i < b.N; i++ {
buf := MakeAioBuffer(bufferSize)
+ s := buf.Slice()
for i := uint(0); i < bufferSize; i++ {
- *buf.Get(i) = 0
+ s[i] = 0
}
buf.Free()
}
}
// Benchmark creating a buffer by copying a Go slice.
func BenchmarkFromBytes(b *testing.B) {
for i := 0; i < b.N; i++ {
zeroes := make([]byte, bufferSize)
buf := FromBytes(zeroes)
diff --git a/golang/libnbd_500_aio_pread_test.go
b/golang/libnbd_500_aio_pread_test.go
index 0811378c..bd0208ef 100644
--- a/golang/libnbd_500_aio_pread_test.go
+++ b/golang/libnbd_500_aio_pread_test.go
@@ -55,14 +55,14 @@ func Test500AioPRead(t *testing.T) {
}
h.Poll(-1)
}
// Expected data.
expected := make([]byte, 512)
for i := 0; i < 512; i += 8 {
binary.BigEndian.PutUint64(expected[i:i+8], uint64(i))
}
- if !bytes.Equal(buf.Bytes(), expected) {
+ if !bytes.Equal(buf.Slice(), expected) {
t.Fatalf("did not read expected data")
}
}
diff --git a/golang/libnbd_510_aio_pwrite_test.go
b/golang/libnbd_510_aio_pwrite_test.go
index 56cdcb05..493159f2 100644
--- a/golang/libnbd_510_aio_pwrite_test.go
+++ b/golang/libnbd_510_aio_pwrite_test.go
@@ -32,23 +32,25 @@ func Test510AioPWrite(t *testing.T) {
"nbdkit", "-s", "--exit-with-parent", "-v",
"memory", "size=512",
})
if err != nil {
t.Fatalf("could not connect: %s", err)
}
/* Write a pattern and read it back. */
buf := MakeAioBuffer(512)
defer buf.Free()
+
+ s := buf.Slice()
for i := 0; i < 512; i += 2 {
- *buf.Get(uint(i)) = 0x55
- *buf.Get(uint(i + 1)) = 0xAA
+ s[i] = 0x55
+ s[i+1] = 0xAA
}
var cookie uint64
cookie, err = h.AioPwrite(buf, 0, nil)
if err != nil {
t.Fatalf("%s", err)
}
for {
var b bool
b, err = h.AioCommandCompleted(cookie)
@@ -62,14 +64,14 @@ func Test510AioPWrite(t *testing.T) {
}
/* We already tested aio_pread, let's just read the data
back in the regular synchronous way. */
buf2 := make([]byte, 512)
err = h.Pread(buf2, 0, nil)
if err != nil {
t.Fatalf("%s", err)
}
- if !bytes.Equal(buf.Bytes(), buf2) {
+ if !bytes.Equal(buf.Slice(), buf2) {
t.Fatalf("did not read back same data as written")
}
}
--
2.34.1
___
Libguestfs mai
[Libguestfs] [PATCH libnbd v2 5/9] golang: aio_buffer.go: Add Slice()
AioBuffer.Bytes() cannot be used for copying images from NBD to other
APis because it copies the entire image. Add a new Slice() function,
creating a slice backed by the underlying buffer.
Using Slice() is efficient, but less safe, like Get(). The returned
slice must be used only before calling Free(). This should not be an
issue with typical code.
Testing shows that Slice() is much faster than Bytes() for typical 256k
buffer:
BenchmarkAioBufferBytes-12 86616 16529 ns/op
BenchmarkAioBufferSlice-12 10 0.4630 ns/op
I modified the aio_copy example to use AioBuffer and compiled 2
versions, one using Bytes() and one using Slice(). When copying 6g fully
allocated image, the version using Slice() is 1.6 times faster.
$ hyperfine -r3 "./aio_copy-bytes $SRC >/dev/null" "./aio_copy-slice $SRC
>/dev/null"
Benchmark 1: ./aio_copy-bytes nbd+unix:///?socket=/tmp/src.sock >/dev/null
Time (mean ± σ): 3.357 s ± 0.039 s[User: 2.656 s, System: 1.162 s]
Range (min … max):3.313 s … 3.387 s3 runs
Benchmark 2: ./aio_copy-slice nbd+unix:///?socket=/tmp/src.sock >/dev/null
Time (mean ± σ): 2.046 s ± 0.009 s[User: 0.423 s, System: 0.892 s]
Range (min … max):2.037 s … 2.055 s3 runs
Summary
'./aio_copy-slice nbd+unix:///?socket=/tmp/src.sock >/dev/null' ran
1.64 ± 0.02 times faster than './aio_copy-bytes
nbd+unix:///?socket=/tmp/src.sock >/dev/null'
When copying a 6g empty image (qemu-nbd sends one hole chunk for every
read), the version using Slice() is 2.6 times faster.
$ hyperfine -r3 "./aio_copy-bytes $SRC >/dev/null" "./aio_copy-slice $SRC
>/dev/null"
Benchmark 1: ./aio_copy-bytes nbd+unix:///?socket=/tmp/src.sock >/dev/null
Time (mean ± σ): 1.210 s ± 0.023 s[User: 1.428 s, System: 0.345 s]
Range (min … max):1.191 s … 1.235 s3 runs
Benchmark 2: ./aio_copy-slice nbd+unix:///?socket=/tmp/src.sock >/dev/null
Time (mean ± σ): 461.4 ms ± 13.1 ms[User: 394.2 ms, System: 76.6 ms]
Range (min … max): 450.6 ms … 476.0 ms3 runs
Summary
'./aio_copy-slice nbd+unix:///?socket=/tmp/src.sock >/dev/null' ran
2.62 ± 0.09 times faster than './aio_copy-bytes
nbd+unix:///?socket=/tmp/src.sock >/dev/null'
Signed-off-by: Nir Soffer
---
golang/aio_buffer.go | 9 ++
golang/libnbd_020_aio_buffer_test.go | 41
2 files changed, 50 insertions(+)
diff --git a/golang/aio_buffer.go b/golang/aio_buffer.go
index d2e6e350..008d9ae0 100644
--- a/golang/aio_buffer.go
+++ b/golang/aio_buffer.go
@@ -71,16 +71,25 @@ func (b *AioBuffer) Free() {
}
}
// Bytes copies the underlying C array to Go allocated memory and return a
// slice. Modifying the returned slice does not modify the underlying buffer
// backing array.
func (b *AioBuffer) Bytes() []byte {
return C.GoBytes(b.P, C.int(b.Size))
}
+// Slice creates a slice backed by the underlying C array. The slice can be
+// used to access or modify the contents of the underlying array. The slice
+// must not be used after caling Free().
+func (b *AioBuffer) Slice() []byte {
+ // See
https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices
+ // TODO: Use unsafe.Slice() when we require Go 1.17.
+ return (*[1<<30]byte)(b.P)[:b.Size:b.Size]
+}
+
// Get returns a pointer to a byte in the underlying C array. The pointer can
// be used to modify the underlying array. The pointer must not be used after
// calling Free().
func (b *AioBuffer) Get(i uint) *byte {
return (*byte)(unsafe.Pointer(uintptr(b.P) + uintptr(i)))
}
diff --git a/golang/libnbd_020_aio_buffer_test.go
b/golang/libnbd_020_aio_buffer_test.go
index b3a2a8d9..4b1c5f93 100644
--- a/golang/libnbd_020_aio_buffer_test.go
+++ b/golang/libnbd_020_aio_buffer_test.go
@@ -44,20 +44,35 @@ func TestAioBuffer(t *testing.T) {
/* Modifying returned slice does not modify the buffer. */
for i := 0; i < len(b); i++ {
b[i] = 42
}
/* Bytes() still returns zeroes. */
if !bytes.Equal(buf.Bytes(), zeroes) {
t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes())
}
+ /* Creating a slice without copying the underlying buffer. */
+ s := buf.Slice()
+ if !bytes.Equal(s, zeroes) {
+ t.Fatalf("Expected %v, got %v", zeroes, s)
+ }
+
+ /* Modifying the slice modifies the underlying buffer. */
+ for i := 0; i < len(s); i++ {
+ s[i] = 42
+ }
+
+ if !bytes.Equal(buf.Slice(), s) {
+ t.Fatalf("Expected %v, got %v", s, buf.Slice())
+ }
+
/* Create another buffer from Go slice. */
buf2 := FromBytes(zeroes)
defer buf2.Free()
if !bytes.Equal(buf2.Bytes(), zeroes) {
t.Fatalf("Expected %v, got %v", zeroes, buf2.Bytes())
}
/* Crated a zeroed buffer. */
buf3 := M
[Libguestfs] [PATCH libnbd v2 9/9] golang: examples: aio_copy: Simplify using AioBuffer
Now that we have an efficient way to use AioBuffer, we don't need the
hacks to create AioBuffer from Go slice.
Benchmarking AioBuffer shows that allocating a 256k buffer is
practically free, so there is no need for the buffer pool. Now we
allocate a new buffer per request, keep it in the command, and free it
when the request is finished.
Signed-off-by: Nir Soffer
---
golang/examples/aio_copy/aio_copy.go | 29 +---
1 file changed, 5 insertions(+), 24 deletions(-)
diff --git a/golang/examples/aio_copy/aio_copy.go
b/golang/examples/aio_copy/aio_copy.go
index b6f5def1..bb20b478 100644
--- a/golang/examples/aio_copy/aio_copy.go
+++ b/golang/examples/aio_copy/aio_copy.go
@@ -37,53 +37,43 @@
// Example:
//
// ./aio_copy nbd+unix:///?socket=/tmp.nbd >/dev/null
//
package main
import (
"container/list"
"flag"
"os"
- "sync"
"syscall"
- "unsafe"
"libguestfs.org/libnbd"
)
var (
// These options give best performance with fast NVMe drive.
requestSize = flag.Uint("request-size", 256*1024, "maximum request size
in bytes")
requests= flag.Uint("requests", 4, "maximum number of requests in
flight")
h *libnbd.Libnbd
// Keeping commands in a queue ensures commands are written in the right
// order, even if they complete out of order. This allows parallel reads
// with non-seekable output.
queue list.List
-
- // Buffer pool allocating buffers as needed and reusing them.
- bufPool = sync.Pool{
- New: func() interface{} {
- return make([]byte, *requestSize)
- },
- }
)
// command keeps state of single AioPread call while the read is handled by
// libnbd, until the command reach the front of the queue and can be writen to
// the output.
type command struct {
- buf[]byte
- length uint
+ buflibnbd.AioBuffer
ready bool
}
func main() {
flag.Parse()
var err error
h, err = libnbd.Create()
if err != nil {
@@ -139,60 +129,51 @@ func waitForCompletion() {
panic(err)
}
if inflightRequests() < start {
break // A read completed.
}
}
}
func startRead(offset uint64, length uint) {
- buf := bufPool.Get().([]byte)
-
- // Keep buffer in command so we can put it back into the pool when the
- // command completes.
- cmd := &command{buf: buf, length: length}
-
- // Create aio buffer from pool buffer to avoid unneeded allocation for
- // every read, and unneeded copy when completing the read.
- abuf := libnbd.AioBuffer{P: unsafe.Pointer(&buf[0]), Size: length}
+ cmd := &command{buf: libnbd.MakeAioBuffer(length)}
args := libnbd.AioPreadOptargs{
CompletionCallbackSet: true,
CompletionCallback: func(error *int) int {
if *error != 0 {
// This is not documented, but *error is errno
value translated
// from the the NBD server error.
err := syscall.Errno(*error).Error()
panic(err)
}
cmd.ready = true
return 1
},
}
- _, err := h.AioPread(abuf, offset, &args)
+ _, err := h.AioPread(cmd.buf, offset, &args)
if err != nil {
panic(err)
}
queue.PushBack(cmd)
}
func readReady() bool {
return queue.Len() > 0 && queue.Front().Value.(*command).ready
}
func finishRead() {
e := queue.Front()
queue.Remove(e)
cmd := e.Value.(*command)
- b := cmd.buf[:cmd.length]
- _, err := os.Stdout.Write(b)
+ _, err := os.Stdout.Write(cmd.buf.Slice())
if err != nil {
panic(err)
}
- bufPool.Put(cmd.buf)
+ cmd.buf.Free()
}
--
2.34.1
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH libnbd v2 7/9] golang: aio_buffer.go: Speed up FromBytes()
Using Slice() we can use builtin copy() instead of a manual loop, which
is 4.6 times faster with a typical 256k buffer:
Before:
BenchmarkFromBytes-12 9806111474 ns/op
After:
BenchmarkFromBytes-12 48193 24106 ns/op
Signed-off-by: Nir Soffer
---
golang/aio_buffer.go | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/golang/aio_buffer.go b/golang/aio_buffer.go
index 008d9ae0..52ea54de 100644
--- a/golang/aio_buffer.go
+++ b/golang/aio_buffer.go
@@ -47,25 +47,22 @@ func MakeAioBuffer(size uint) AioBuffer {
// MakeAioBuffer makes a new buffer backed by a C allocated array. The
// underlying buffer is set to zero.
func MakeAioBufferZero(size uint) AioBuffer {
return AioBuffer{C.calloc(C.ulong(1), C.ulong(size)), size}
}
// FromBytes makes a new buffer backed by a C allocated array, initialized by
// copying the given Go slice.
func FromBytes(buf []byte) AioBuffer {
- size := len(buf)
- ret := MakeAioBuffer(uint(size))
- for i := 0; i < len(buf); i++ {
- *ret.Get(uint(i)) = buf[i]
- }
+ ret := MakeAioBuffer(uint(len(buf)))
+ copy(ret.Slice(), buf)
return ret
}
// Free deallocates the underlying C allocated array. Using the buffer after
// Free() will panic.
func (b *AioBuffer) Free() {
if b.P != nil {
C.free(b.P)
b.P = nil
}
--
2.34.1
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH libnbd v2 8/9] golang: aio_buffer.go: Benchmark copy flows
Add benchmark for coping a buffer using 3 strategies - reusing same
buffer, making a new uninitialized buffer per copy, and using a zeroed
buffer per copy. This benchmark is the worst possible case, copying a
buffer to memory. Any real I/O will be much slower, hiding the overhead
of allocating or zeroing buffers.
$ go test -run=AioBuffer -bench=Copy -benchtime=5s
goos: linux
goarch: amd64
pkg: libguestfs.org/libnbd
cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz
BenchmarkAioBufferCopyBaseline-121142508 4523 ns/op
BenchmarkAioBufferCopyMake-12100 5320 ns/op
BenchmarkAioBufferCopyMakeZero-12 728940 8218 ns/op
Signed-off-by: Nir Soffer
---
golang/libnbd_020_aio_buffer_test.go | 32
1 file changed, 32 insertions(+)
diff --git a/golang/libnbd_020_aio_buffer_test.go
b/golang/libnbd_020_aio_buffer_test.go
index e07f8973..f38866e7 100644
--- a/golang/libnbd_020_aio_buffer_test.go
+++ b/golang/libnbd_020_aio_buffer_test.go
@@ -195,10 +195,42 @@ func BenchmarkAioBufferBytes(b *testing.B) {
func BenchmarkAioBufferSlice(b *testing.B) {
buf := MakeAioBuffer(bufferSize)
defer buf.Free()
var r int
b.ResetTimer()
for i := 0; i < b.N; i++ {
r += len(buf.Slice())
}
}
+
+var data = make([]byte, bufferSize)
+
+// Benchmark copying into same buffer, used as baseline for CopyMake and
+// CopyMakeZero benchmarks.
+func BenchmarkAioBufferCopyBaseline(b *testing.B) {
+ buf := MakeAioBufferZero(bufferSize)
+ defer buf.Free()
+
+ b.ResetTimer()
+ for i := 0; i < b.N; i++ {
+ copy(buf.Slice(), data)
+ }
+}
+
+// Benchmark overhead of making a new buffer per read.
+func BenchmarkAioBufferCopyMake(b *testing.B) {
+ for i := 0; i < b.N; i++ {
+ buf := MakeAioBuffer(bufferSize)
+ copy(buf.Slice(), data)
+ buf.Free()
+ }
+}
+
+// Benchmark overhead of making a new zero buffer per read.
+func BenchmarkAioBufferCopyMakeZero(b *testing.B) {
+ for i := 0; i < b.N; i++ {
+ buf := MakeAioBufferZero(bufferSize)
+ copy(buf.Slice(), data)
+ buf.Free()
+ }
+}
--
2.34.1
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH libnbd v2 4/9] golang: aio_buffer.go: Add MakeAioBufferZero()
Make it easy to create a zeroed buffer via calloc(), preventing leaking
sensitive info from the heap.
Benchmarking shows that creating a zeroed buffer is much slower compared
with uninitialized buffer, but much faster compared with manually
initializing the buffer with a loop.
BenchmarkMakeAioBuffer-127252674 148.1 ns/op
BenchmarkMakeAioBufferZero-12 262107 4181 ns/op
BenchmarkAioBufferZero-12 17581 68759 ns/op
It is interesting that creating a zeroed buffer is 3 times faster
compared with making a new []byte slice:
BenchmarkMakeAioBufferZero-12 247710 4440 ns/op
BenchmarkMakeByteSlice-12 84117 13733 ns/op
Signed-off-by: Nir Soffer
---
golang/aio_buffer.go | 6 ++
golang/libnbd_020_aio_buffer_test.go | 16
2 files changed, 22 insertions(+)
diff --git a/golang/aio_buffer.go b/golang/aio_buffer.go
index 2cd8ceb2..d2e6e350 100644
--- a/golang/aio_buffer.go
+++ b/golang/aio_buffer.go
@@ -38,20 +38,26 @@ type AioBuffer struct {
Punsafe.Pointer
Size uint
}
// MakeAioBuffer makes a new buffer backed by an uninitialized C allocated
// array.
func MakeAioBuffer(size uint) AioBuffer {
return AioBuffer{C.malloc(C.ulong(size)), size}
}
+// MakeAioBuffer makes a new buffer backed by a C allocated array. The
+// underlying buffer is set to zero.
+func MakeAioBufferZero(size uint) AioBuffer {
+ return AioBuffer{C.calloc(C.ulong(1), C.ulong(size)), size}
+}
+
// FromBytes makes a new buffer backed by a C allocated array, initialized by
// copying the given Go slice.
func FromBytes(buf []byte) AioBuffer {
size := len(buf)
ret := MakeAioBuffer(uint(size))
for i := 0; i < len(buf); i++ {
*ret.Get(uint(i)) = buf[i]
}
return ret
}
diff --git a/golang/libnbd_020_aio_buffer_test.go
b/golang/libnbd_020_aio_buffer_test.go
index cec74ddc..b3a2a8d9 100644
--- a/golang/libnbd_020_aio_buffer_test.go
+++ b/golang/libnbd_020_aio_buffer_test.go
@@ -51,20 +51,28 @@ func TestAioBuffer(t *testing.T) {
t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes())
}
/* Create another buffer from Go slice. */
buf2 := FromBytes(zeroes)
defer buf2.Free()
if !bytes.Equal(buf2.Bytes(), zeroes) {
t.Fatalf("Expected %v, got %v", zeroes, buf2.Bytes())
}
+
+ /* Crated a zeroed buffer. */
+ buf3 := MakeAioBufferZero(uint(32))
+ defer buf.Free()
+
+ if !bytes.Equal(buf3.Bytes(), zeroes) {
+ t.Fatalf("Expected %v, got %v", zeroes, buf2.Bytes())
+ }
}
func TestAioBufferFree(t *testing.T) {
buf := MakeAioBuffer(uint(32))
/* Free the underlying C array. */
buf.Free()
/* And clear the pointer. */
if buf.P != nil {
@@ -105,20 +113,28 @@ func TestAioBufferGetAfterFree(t *testing.T) {
const bufferSize uint = 256 * 1024
// Benchmark creating an uninitialized buffer.
func BenchmarkMakeAioBuffer(b *testing.B) {
for i := 0; i < b.N; i++ {
buf := MakeAioBuffer(bufferSize)
buf.Free()
}
}
+// Benchmark creating zeroed buffer.
+func BenchmarkMakeAioBufferZero(b *testing.B) {
+ for i := 0; i < b.N; i++ {
+ buf := MakeAioBufferZero(bufferSize)
+ buf.Free()
+ }
+}
+
// Benchmark zeroing a buffer.
func BenchmarkAioBufferZero(b *testing.B) {
for i := 0; i < b.N; i++ {
buf := MakeAioBuffer(bufferSize)
for i := uint(0); i < bufferSize; i++ {
*buf.Get(i) = 0
}
buf.Free()
}
}
--
2.34.1
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH libnbd v2 3/9] golang: aio_buffer.go: Add missing documentation
Add standard function documentation comments.
The documentation should be available here:
https://pkg.go.dev/libguestfs.org/libnbd#AioBuffer
Signed-off-by: Nir Soffer
---
golang/aio_buffer.go | 12
1 file changed, 12 insertions(+)
diff --git a/golang/aio_buffer.go b/golang/aio_buffer.go
index 2b77d6ee..2cd8ceb2 100644
--- a/golang/aio_buffer.go
+++ b/golang/aio_buffer.go
@@ -32,37 +32,49 @@ package libnbd
import "C"
import "unsafe"
/* Asynchronous I/O buffer. */
type AioBuffer struct {
Punsafe.Pointer
Size uint
}
+// MakeAioBuffer makes a new buffer backed by an uninitialized C allocated
+// array.
func MakeAioBuffer(size uint) AioBuffer {
return AioBuffer{C.malloc(C.ulong(size)), size}
}
+// FromBytes makes a new buffer backed by a C allocated array, initialized by
+// copying the given Go slice.
func FromBytes(buf []byte) AioBuffer {
size := len(buf)
ret := MakeAioBuffer(uint(size))
for i := 0; i < len(buf); i++ {
*ret.Get(uint(i)) = buf[i]
}
return ret
}
+// Free deallocates the underlying C allocated array. Using the buffer after
+// Free() will panic.
func (b *AioBuffer) Free() {
if b.P != nil {
C.free(b.P)
b.P = nil
}
}
+// Bytes copies the underlying C array to Go allocated memory and return a
+// slice. Modifying the returned slice does not modify the underlying buffer
+// backing array.
func (b *AioBuffer) Bytes() []byte {
return C.GoBytes(b.P, C.int(b.Size))
}
+// Get returns a pointer to a byte in the underlying C array. The pointer can
+// be used to modify the underlying array. The pointer must not be used after
+// calling Free().
func (b *AioBuffer) Get(i uint) *byte {
return (*byte)(unsafe.Pointer(uintptr(b.P) + uintptr(i)))
}
--
2.34.1
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH libnbd v2 0/9] golang: Safer, easier to use, and faster AioBuffer
Improve AioBuffer to make it safer, easier to use, and faster when integrating with other Go APIs. New Go specific APIs: - MakeAioBufferZero() - creates a new buffer using calloc(), to make it easy and efficient to use a zeroed buffer. - AioBuffer.Slice() - create a slice backed by the underlying buffer without copying the contents of the buffer. Performance improments: - FromBytes() is 3 time faster - Code using Bytes() should use Slice() now. aio_copy example shows up to 260% speedup. Improve testing: - New AioBuffer tests - New AioBuffer benchmarks Documention: - AioBuffer is fully documnted now. Changes since v1: - Rename the new test to libnbd_020_ to match the current test numbering semantics (Eric) - We run the benchmarks in make check using very short timeout to keep them working without overloading the CI. (Eric) - Update copyright year (Eric) - Fix many typos in comments and commit messages (Eric) v1 was here: https://listman.redhat.com/archives/libguestfs/2022-January/msg00218.html Nir Soffer (9): golang: tests: Add test for AioBuffer golang: aio_buffer.go: Make it safer to use golang: aio_buffer.go: Add missing documentation golang: aio_buffer.go: Add MakeAioBufferZero() golang: aio_buffer.go: Add Slice() golang: tests: Use AioBuffer.Slice() golang: aio_buffer.go: Speed up FromBytes() golang: aio_buffer.go: Benchmark copy flows golang: examples: aio_copy: Simplify using AioBuffer golang/Makefile.am | 1 + golang/aio_buffer.go | 39 - golang/examples/aio_copy/aio_copy.go | 29 +--- golang/libnbd_020_aio_buffer_test.go | 236 +++ golang/libnbd_500_aio_pread_test.go | 2 +- golang/libnbd_510_aio_pwrite_test.go | 8 +- golang/run-tests.sh | 5 + 7 files changed, 286 insertions(+), 34 deletions(-) create mode 100644 golang/libnbd_020_aio_buffer_test.go -- 2.34.1 ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH libnbd v2 2/9] golang: aio_buffer.go: Make it safer to use
If a Go program tries to use AioBuffer after calling AioBuffer.Free(),
the program may silently corrupt data, accessing memory that does not
belong to the buffer any more, or segfault if the address is not mapped.
In the worst case, it can corrupt memory silently. Calling Free() twice
may silently free unrelated memory.
Make the buffer safer to use by Freeing only on the first call and
setting the pointer to nil. This makes multiple calls to Free()
harmless, just like the underlying C.free().
Trying to access Bytes() and Get() after calling Free() will always
panic now, revealing the bug in the program.
Trying to use AioBuffer with libnbd API will likely segfault and panic.
I did not try to test this.
Signed-off-by: Nir Soffer
---
golang/aio_buffer.go | 5 +++-
golang/libnbd_020_aio_buffer_test.go | 41
2 files changed, 45 insertions(+), 1 deletion(-)
diff --git a/golang/aio_buffer.go b/golang/aio_buffer.go
index 2bc69a01..2b77d6ee 100644
--- a/golang/aio_buffer.go
+++ b/golang/aio_buffer.go
@@ -46,20 +46,23 @@ func MakeAioBuffer(size uint) AioBuffer {
func FromBytes(buf []byte) AioBuffer {
size := len(buf)
ret := MakeAioBuffer(uint(size))
for i := 0; i < len(buf); i++ {
*ret.Get(uint(i)) = buf[i]
}
return ret
}
func (b *AioBuffer) Free() {
- C.free(b.P)
+ if b.P != nil {
+ C.free(b.P)
+ b.P = nil
+ }
}
func (b *AioBuffer) Bytes() []byte {
return C.GoBytes(b.P, C.int(b.Size))
}
func (b *AioBuffer) Get(i uint) *byte {
return (*byte)(unsafe.Pointer(uintptr(b.P) + uintptr(i)))
}
diff --git a/golang/libnbd_020_aio_buffer_test.go
b/golang/libnbd_020_aio_buffer_test.go
index 5898746b..cec74ddc 100644
--- a/golang/libnbd_020_aio_buffer_test.go
+++ b/golang/libnbd_020_aio_buffer_test.go
@@ -53,20 +53,61 @@ func TestAioBuffer(t *testing.T) {
/* Create another buffer from Go slice. */
buf2 := FromBytes(zeroes)
defer buf2.Free()
if !bytes.Equal(buf2.Bytes(), zeroes) {
t.Fatalf("Expected %v, got %v", zeroes, buf2.Bytes())
}
}
+func TestAioBufferFree(t *testing.T) {
+ buf := MakeAioBuffer(uint(32))
+
+ /* Free the underlying C array. */
+ buf.Free()
+
+ /* And clear the pointer. */
+ if buf.P != nil {
+ t.Fatal("Dangling pointer after Free()")
+ }
+
+ /* Additional Free does nothing. */
+ buf.Free()
+}
+
+func TestAioBufferBytesAfterFree(t *testing.T) {
+ buf := MakeAioBuffer(uint(32))
+ buf.Free()
+
+ defer func() {
+ if r := recover(); r == nil {
+ t.Fatal("Did not recover from panic calling Bytes()
after Free()")
+ }
+ }()
+
+ buf.Bytes()
+}
+
+func TestAioBufferGetAfterFree(t *testing.T) {
+ buf := MakeAioBuffer(uint(32))
+ buf.Free()
+
+ defer func() {
+ if r := recover(); r == nil {
+ t.Fatal("Did not recover from panic calling Get() after
Free()")
+ }
+ }()
+
+ *buf.Get(0) = 42
+}
+
// Typical buffer size.
const bufferSize uint = 256 * 1024
// Benchmark creating an uninitialized buffer.
func BenchmarkMakeAioBuffer(b *testing.B) {
for i := 0; i < b.N; i++ {
buf := MakeAioBuffer(bufferSize)
buf.Free()
}
}
--
2.34.1
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH libnbd v2 1/9] golang: tests: Add test for AioBuffer
Add unit tests and benchmarks for AioBuffer. The tests are trivial but
they server as running documentation, and they point out important
details about the type.
The benchmarks show the efficiency of allocating a new buffer, zeroing
it, and interfacing with Go code.
These tests will also ensure that we don't break anything by the next
changes.
To run the benchmarks use:
$ go test -run=xxx -bench=.
goos: linux
goarch: amd64
pkg: libguestfs.org/libnbd
cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz
BenchmarkMakeAioBuffer-126871759 157.2 ns/op
BenchmarkAioBufferZero-12 17551 69552 ns/op
BenchmarkFromBytes-12 9632139112 ns/op
BenchmarkAioBufferBytes-12 69375 16410 ns/op
PASS
ok libguestfs.org/libnbd 5.843s
To make sure the benchmarks will not break, we run them in "make check"
with a very short timeout. For actual performance testing run "go test"
directly.
Signed-off-by: Nir Soffer
---
golang/Makefile.am | 1 +
golang/libnbd_020_aio_buffer_test.go | 104 +++
golang/run-tests.sh | 5 ++
3 files changed, 110 insertions(+)
create mode 100644 golang/libnbd_020_aio_buffer_test.go
diff --git a/golang/Makefile.am b/golang/Makefile.am
index 10fb8934..f170cbc4 100644
--- a/golang/Makefile.am
+++ b/golang/Makefile.am
@@ -19,20 +19,21 @@ include $(top_srcdir)/subdir-rules.mk
source_files = \
aio_buffer.go \
bindings.go \
callbacks.go \
closures.go \
handle.go \
wrappers.go \
wrappers.h \
libnbd_010_load_test.go \
+ libnbd_020_aio_buffer_test.go \
libnbd_100_handle_test.go \
libnbd_110_defaults_test.go \
libnbd_120_set_non_defaults_test.go \
libnbd_200_connect_command_test.go \
libnbd_210_opt_abort_test.go \
libnbd_220_opt_list_test.go \
libnbd_230_opt_info_test.go \
libnbd_240_opt_list_meta_test.go \
libnbd_300_get_size_test.go \
libnbd_400_pread_test.go \
diff --git a/golang/libnbd_020_aio_buffer_test.go
b/golang/libnbd_020_aio_buffer_test.go
new file mode 100644
index ..5898746b
--- /dev/null
+++ b/golang/libnbd_020_aio_buffer_test.go
@@ -0,0 +1,104 @@
+/* libnbd golang tests
+ * Copyright (C) 2013-2022 Red Hat Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+package libnbd
+
+import (
+ "bytes"
+ "testing"
+)
+
+func TestAioBuffer(t *testing.T) {
+ /* Create a buffer with uninitialized backing array. */
+ buf := MakeAioBuffer(uint(32))
+ defer buf.Free()
+
+ /* Initialize backing array contents. */
+ for i := uint(0); i < buf.Size; i++ {
+ *buf.Get(i) = 0
+ }
+
+ /* Create a slice by copying the backing array contents into Go memory.
*/
+ b := buf.Bytes()
+
+ zeroes := make([]byte, 32)
+ if !bytes.Equal(b, zeroes) {
+ t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes())
+ }
+
+ /* Modifying returned slice does not modify the buffer. */
+ for i := 0; i < len(b); i++ {
+ b[i] = 42
+ }
+
+ /* Bytes() still returns zeroes. */
+ if !bytes.Equal(buf.Bytes(), zeroes) {
+ t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes())
+ }
+
+ /* Create another buffer from Go slice. */
+ buf2 := FromBytes(zeroes)
+ defer buf2.Free()
+
+ if !bytes.Equal(buf2.Bytes(), zeroes) {
+ t.Fatalf("Expected %v, got %v", zeroes, buf2.Bytes())
+ }
+}
+
+// Typical buffer size.
+const bufferSize uint = 256 * 1024
+
+// Benchmark creating an uninitialized buffer.
+func BenchmarkMakeAioBuffer(b *testing.B) {
+ for i := 0; i < b.N; i++ {
+ buf := MakeAioBuffer(bufferSize)
+ buf.Free()
+ }
+}
+
+// Benchmark zeroing a buffer.
+func BenchmarkAioBufferZero(b *testing.B) {
+ for i := 0; i < b.N; i++ {
+ buf := MakeAioBuffer(bufferSize)
+ for i := uint(0); i < bufferSize; i++ {
+ *buf.Get(i) = 0
+ }
+ buf.Free()
+ }
+}
+
+// Benchmark creating a buffer by copying a Go slice.
+func BenchmarkFr
Re: [Libguestfs] [PATCH libnbd 5/9] golang: aio_buffer.go: Add Slice()
On Tue, Feb 1, 2022 at 3:12 PM Eric Blake wrote:
>
> On Sun, Jan 30, 2022 at 01:33:33AM +0200, Nir Soffer wrote:
> > AioBuffer.Bytes() cannot be used for coping images from NBD to other
>
> copying
>
> > APis because it copies the entire image. Add a new Slice() function,
> > creating a slice backed by the underling buffer.
>
> underlying
>
> >
> > Using Slice() is efficient, but less safe, like Get(). The returned
> > slice must be used only before calling Free(). This should not be an
> > issue with typical code.
> >
> > Testing show that Slice() is much faster than Bytes() for typical 256k
> > buffer:
> >
> > BenchmarkAioBufferBytes-12 86616 16529 ns/op
> > BenchmarkAioBufferSlice-1210 0.4630 ns/op
> >
> > I modified the aio_copy example to use AioBuffer and complied 2
>
> compiled
>
> > +++ b/golang/libnbd_620_aio_buffer_test.go
> > @@ -44,20 +44,35 @@ func TestAioBuffer(t *testing.T) {
> > /* Modifying returned slice does not modify the buffer. */
> > for i := 0; i < len(b); i++ {
> > b[i] = 42
> > }
> >
> > /* Bytes() still returns zeroes. */
> > if !bytes.Equal(buf.Bytes(), zeroes) {
> > t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes())
> > }
> >
> > + /* Creating a slice without copying the underlhing buffer. */
>
> underlying
>
> > + s := buf.Slice()
> > + if !bytes.Equal(s, zeroes) {
> > + t.Fatalf("Expected %v, got %v", zeroes, s)
> > + }
> > +
> > + /* Modifing the slice modifies the underlying buffer. */
>
> Modifying
>
> > }
> > +
> > +// Benchmark creating a slice without copying the underling buffer.
>
> underlying
>
> > +func BenchmarkAioBufferSlice(b *testing.B) {
> > + buf := MakeAioBuffer(bufferSize)
> > + defer buf.Free()
> > + var r int
> > +
> > + b.ResetTimer()
> > + for i := 0; i < b.N; i++ {
> > + r += len(buf.Slice())
> > + }
> > +}
> > --
> > 2.34.1
Will fix in v2
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH libnbd 4/9] golang: aio_buffer.go: Add MakeAioBufferZero()
On Tue, Feb 8, 2022 at 10:45 PM Eric Blake wrote: > > On Sun, Jan 30, 2022 at 01:33:32AM +0200, Nir Soffer wrote: > > Make it easy to create a zeroed buffer via calloc(), preventing leaking > > sensitive info from the heap. > > > > Benchmarking show that creating a zeroed buffer is much slower compared > > shows Will fix > > > with uninitialized buffer, but much faster compared with manually > > initializing the buffer with a loop. > > > > BenchmarkMakeAioBuffer-12 7252674 148.1 ns/op > > BenchmarkMakeAioBufferZero-12 262107 4181 ns/op > > BenchmarkAioBufferZero-1217581 68759 ns/op > > > > It is interesting that creating a zeroed buffer is 3 times faster > > compared with making a new []byte slice: > > > > BenchmarkMakeAioBufferZero-12 247710 4440 ns/op > > BenchmarkMakeByteSlice-1284117 13733 ns/op > > Some of this is due to how much vectorization the standard library > (whether libc or Go's core libraries) can do when bulk-zeroing > (zeroing 64 bits, or even a cache line at a time, in an unrolled loop, > is always going to be more performant than a naive loop of one byte at > a time). > > > > > Signed-off-by: Nir Soffer > > --- > > golang/aio_buffer.go | 6 ++ > > golang/libnbd_620_aio_buffer_test.go | 16 > > Another file that may fit better in the 0xx naming, especially if we > decide to duplicate similar functionality into the python or OCaml > bindings of being able to pre-generate a known-zero buffer for use in > passing to nbd_pread. > > As a helper API, this seems useful. But do we need any man page > documentation of a language-specific helper function? The AioBuffer type is documented here: https://pkg.go.dev/libguestfs.org/libnbd#AioBuffer Patch #3 golang: aio_buffer.go: Add missing documentation adds the missing documentation for the functions, We can add more documentation for the type. If we need task-based documentation I think improving libnbd-golang is the best way. ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH libnbd 3/9] golang: aio_buffer.go: Add missing documentation
On Tue, Feb 1, 2022 at 3:09 PM Eric Blake wrote:
>
> On Sun, Jan 30, 2022 at 01:33:31AM +0200, Nir Soffer wrote:
> > Add standard function documentation comments.
> >
> > The documentation should be available here:
> > https://pkg.go.dev/libguestfs.org/libnbd#AioBuffer
> >
> > Signed-off-by: Nir Soffer
> > ---
>
> >
> > +// MakeAioBuffer makes a new buffer backed by an unitilialized C allocated
>
> uninitialized
>
> > +// array.
> > func MakeAioBuffer(size uint) AioBuffer {
> > return AioBuffer{C.malloc(C.ulong(size)), size}
> > }
> >
> > +// FromBytes makes a new buffer backed by a C allocated array, initialized
> > by
> > +// copying the given Go slice.
> > func FromBytes(buf []byte) AioBuffer {
> > size := len(buf)
> > ret := MakeAioBuffer(uint(size))
> > for i := 0; i < len(buf); i++ {
> > *ret.Get(uint(i)) = buf[i]
> > }
> > return ret
> > }
> >
> > +// Free deallocates the underlying C allocated array. Using the buffer
> > after
> > +// Free() will panic.
> > func (b *AioBuffer) Free() {
> > if b.P != nil {
> > C.free(b.P)
> > b.P = nil
> > }
> > }
> >
> > +// Bytes copies the underlying C array to Go allocated memory and return a
> > +// slice. Modifying the returned slice does not modify the unerlying buffer
>
> underlying
>
> > +// backking array.
>
> backing
>
> > func (b *AioBuffer) Bytes() []byte {
> > return C.GoBytes(b.P, C.int(b.Size))
> > }
> >
> > +// Get returns a pointer to a byte in the underlying C array. The pointer
> > can
> > +// be used to modify the underlying array. The pointer must not be used
> > after
> > +// caling Free().
> > func (b *AioBuffer) Get(i uint) *byte {
> > return (*byte)(unsafe.Pointer(uintptr(b.P) + uintptr(i)))
> > }
> > --
> > 2.34.1
Thanks, will update in v2
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH libnbd 1/9] golang: tests: Add test for AioBuffer
On Tue, Feb 8, 2022 at 9:33 PM Eric Blake wrote:
>
> On Sun, Jan 30, 2022 at 01:33:29AM +0200, Nir Soffer wrote:
> > Add unit tests and benchmarks for AioBuffer. The tests are trivial but
> > they server as running documentation, and they point out important
> > details about the type.
> >
> > The benchmarks how efficient is allocation a new buffer, zeroing it, and
> > interfacing with Go code.
>
> Wording suggestion:
>
> The benchmarks show the efficiency of allocating a new buffer, zeroing
> it, and interfacing with Go code.
Thanks, I will use this.
> >
> > This tests will also ensure that we don't break anything by the next
>
> Either "These tests" or "This test"
Right
> > changes.
> >
> > To run the benchmarks use:
> >
> > $ go test -run=xxx -bench=.
> > goos: linux
> > goarch: amd64
> > pkg: libguestfs.org/libnbd
> > cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz
> > BenchmarkMakeAioBuffer-12 6871759 157.2 ns/op
> > BenchmarkAioBufferZero-1217551 69552 ns/op
> > BenchmarkFromBytes-12 9632139112 ns/op
> > BenchmarkAioBufferBytes-12 69375 16410 ns/op
> > PASS
> > oklibguestfs.org/libnbd 5.843s
> >
> > Signed-off-by: Nir Soffer
> > ---
> > golang/Makefile.am | 1 +
> > golang/libnbd_620_aio_buffer_test.go | 104 +++
> > 2 files changed, 105 insertions(+)
> > create mode 100644 golang/libnbd_620_aio_buffer_test.go
> >
> > diff --git a/golang/Makefile.am b/golang/Makefile.am
> > index 10fb8934..ae0486dd 100644
> > --- a/golang/Makefile.am
> > +++ b/golang/Makefile.am
> > @@ -37,20 +37,21 @@ source_files = \
> > libnbd_300_get_size_test.go \
> > libnbd_400_pread_test.go \
> > libnbd_405_pread_structured_test.go \
> > libnbd_410_pwrite_test.go \
> > libnbd_460_block_status_test.go \
> > libnbd_500_aio_pread_test.go \
> > libnbd_510_aio_pwrite_test.go \
> > libnbd_590_aio_copy_test.go \
> > libnbd_600_debug_callback_test.go \
> > libnbd_610_error_test.go \
> > + libnbd_620_aio_buffer_test.go \
>
> As discussed in a different thread, the numbering here groups
> somewhat-related functionality, and helps us keep cross-language tests
> correlated over testing the same features. Since you aren't adding
> counterpart tests to python or ocaml, I don't know what number would
> be best. But our existing numbering is more along the lines of 0xx
> for language-level loading, 1xx for NBD handle tests, 2xx for
> connection tests, 3xx for initial APIs after connecting, 4xx for
> synchronous APIs, 5xx for asynch APIs, and 6xx for high-level usage
> patterns. This feels like it might fit better in the 0xx series,
> since the benchmark does not use any NBD handle.
I agree. When I posted this I did not understand the semantics
and assumed the numbers reflect the order the tests were added.
I'll add this to the 0xx group.
>
> > $(NULL)
> >
> > generator_built = \
> > bindings.go \
> > closures.go \
> > wrappers.go \
> > wrappers.h \
> > $(NULL)
> >
> > EXTRA_DIST = \
> > diff --git a/golang/libnbd_620_aio_buffer_test.go
> > b/golang/libnbd_620_aio_buffer_test.go
> > new file mode 100644
> > index ..2632f87f
> > --- /dev/null
> > +++ b/golang/libnbd_620_aio_buffer_test.go
> > @@ -0,0 +1,104 @@
> > +/* libnbd golang tests
> > + * Copyright (C) 2013-2021 Red Hat Inc.
>
> You may want to add 2022.
>
> Take the rest of my review with a grain of salt; I'm not (yet?) a Go expert.
>
> > +
> > +package libnbd
> > +
> > +import (
> > + "bytes"
> > + "testing"
> > +)
> > +
> > +func TestAioBuffer(t *testing.T) {
> > + /* Create a buffer with unitinialized backing array. */
>
> uninitialized
>
> > + buf := MakeAioBuffer(uint(32))
> > + defer buf.Free()
> > +
> > + /* Initialize backing array contents. */
> > + for i := uint(0); i < buf.Size; i++ {
> > + *buf.Get(i) = 0
> > + }
> > +
> > + /* Create a slice by copying the backing arrary contents into Go
> > memory. */
> > + b := buf.Bytes()
> > +
> > + zeroes := make([]byte, 32)
> > + if !bytes.Equal(b, zeroes) {
> > + t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes())
> > + }
> > +
> > + /* Modifying returned slice does not modify the buffer. */
> > + for i := 0; i < len(b); i++ {
> > + b[i] = 42
> > + }
> > +
> > + /* Bytes() still returns zeroes. */
> > + if !bytes.Equal(buf.Bytes(), zeroes) {
> > + t.Fatalf("Expected %v, got %v", zeroes, buf.Bytes())
> > + }
> > +
> > + /* Create a nother buffer from Go slice. */
>
> another
>
> > + buf2 := FromBytes(zeroes)
> > + defer buf2.Free()
> > +
> > + if !bytes.Equal(buf2.Bytes(), zeroes) {
> > + t.Fatalf("Expected %v, got %v", zeroes, buf2.Bytes())
> > + }
> > +}
> > +
> > +// Typical buffer size.
> > +co
Re: [Libguestfs] [PATCH libnbd] generator/Go.ml: Simplify copy_uint32_array
On Tue, Feb 8, 2022 at 9:23 PM Eric Blake wrote: > > On Sun, Feb 06, 2022 at 07:45:20PM +0200, Nir Soffer wrote: > > Create a slice backed up by the entries pointer, and copy the data with > > builtin copy(). This can be 3x times faster but I did not measure it. > > > > Eric posted a similar patch[1] last year, but the patch seems to be > > stuck with unrelated incomplete work. > > > > [1] > > https://listman.redhat.com/archives/libguestfs/2021-December/msg00065.html > > Your version looks slightly nicer than mine. ACK. Thanks, pushed as 6725fa0e129f9a60d7b89707ef8604e0aeeeaf43 ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH 5/5] output/rhv-upload-plugin: Keep connections alive
On Tue, Feb 8, 2022 at 5:24 PM Richard W.M. Jones wrote: ... > > ACK 4 & 5. Push series without patch 2 as 99b6e31b output/rhv-upload-plugin: Keep connections alive 02d2236b output/rhv-upload-plugin: Track http last request time a436a0dc output/rhv-upload-plugin: Extract send_flush() helper 6e4f3270 output/rhv-upload-plugin: Fix flush and close ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH 3/3] api: Add new API nbd_set_pread_initialize()
On Thu, Feb 10, 2022 at 03:09:46PM +, Richard W.M. Jones wrote: > For Fedora, I won't do anything now, just let this go out in the next > release. It'll need a small adjustment if we backport it to the > stable-1.10 branch. > > For RHEL, I'm going to put this in RHEL 9.0, but won't bother with > RHEL 8 (it has ancient history libnbd 1.6). So what I actually did and why: Upstream: backported the first 2 patches and not the new API. Very minor performance regression, but we don't have awkward questions about whether the new API was introduced in 1.10 or 1.12 (because if we do add it in 1.10, then it confuses people using <= 1.10.3). Fedora: same as upstream. RHEL 9.0: backported all 3 patches, including the new API. Adjusted the message downstream to make it appear the new API is added in 1.10. RHEL 8: nothing Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/ ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [nbdkit PATCH] nbd: Opt in to libnbd pread_initialize speedup
Our nbd plugin has always properly checked for asynch errors (and thus
has never been at risk of a vulnerability similar to CVE-2022-0485
just fixed in nbdcopy). What's more, the nbdkit core guarantees
(since commit b1ce255e in 2019, v1.13.1) that the buffer handed to a
.pread callback has been pre-sanitized to not leak heap contents,
regardless of how buggy a plugin might be (see
server/protocol.c:protocol_recv_request_send_reply(), which uses
server/threadlocal.c:threadlocal_buffer() for a safe buffer). Thus,
we do not need libnbd to spend time re-initializing the buffer, if
libnbd is new enough to give us that knob.
---
I know that Laszlo was sceptical whether any real client might want to
use the libnbd knob [1], but I'm actually comfortable with this one.
[1] https://listman.redhat.com/archives/libguestfs/2022-February/msg00144.html
plugins/nbd/nbd.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
index ae595ea7..2e154e8d 100644
--- a/plugins/nbd/nbd.c
+++ b/plugins/nbd/nbd.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2017-2020, 2022 Red Hat Inc.
+ * Copyright (C) 2017-2022 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -637,6 +637,15 @@ nbdplug_open_handle (int readonly, const char
*client_export)
#if LIBNBD_HAVE_NBD_SET_FULL_INFO
if (nbd_set_full_info (h->nbd, 1) == -1)
goto errnbd;
+#endif
+#if LIBNBD_HAVE_NBD_SET_PREAD_INITIALIZE
+ /* nbdkit guarantees that the buffers passed to our .pread callback
+ * are pre-initialized; and we in turn ensure that the buffer is not
+ * dereferenced if the NBD server replied with an error. Thus, we
+ * are safe opting in to this libnbd speedup.
+ */
+ if (nbd_set_pread_initialize (h->nbd, false) == -1)
+goto errnbd;
#endif
if (dynamic_export && uri) {
#if LIBNBD_HAVE_NBD_SET_OPT_MODE
--
2.34.1
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH 3/3] api: Add new API nbd_set_pread_initialize()
On Thu, Feb 10, 2022 at 08:50:07AM -0600, Eric Blake wrote:
> On Thu, Feb 10, 2022 at 09:38:30AM +, Richard W.M. Jones wrote:
> > On Wed, Feb 09, 2022 at 04:07:26PM -0600, Eric Blake wrote:
> > > + "set_pread_initialize", {
> > > +default_call with
> > > +args = [Bool "request"]; ret = RErr;
> > > +shortdesc = "control whether libnbd pre-initializes read buffers";
> > > +longdesc = "\
> > > +By default, libnbd will pre-initialize the contents of a buffer
> > > +passed to calls such as L to all zeroes prior to checking
> > > +for any other errors, so that even if a client application passed in an
> > > +uninitialized buffer but fails to check for errors, it will not result
> > > +in a potential security risk caused by an accidental leak of prior heap
> > > +contents. However, for a client application that has audited that an
> > > +uninitialized buffer is never dereferenced, or which performs its own
> > > +pre-initialization, libnbd's sanitization efforts merely pessimize
> > > +performance.
> > > +
> > > +Calling this function with C set to false tells libnbd to
> > > +skip the buffer initialization step in read commands.";
> > > +see_also = [Link "get_pread_initialize";
> > > +Link "set_strict_mode";
> > > +Link "pread"; Link "pread_structured"; Link "aio_pread";
> > > +Link "aio_pread_structured"];
> > > + };
> >
> > Could it be worth mentioning CVE-2022-0485 by name in the text here?
> > And/or linking to:
> > https://listman.redhat.com/archives/libguestfs/2022-February/msg00104.html
>
> Good idea, I went with:
>
> diff --git i/generator/API.ml w/generator/API.ml
> index 9b7eb545..aaba9951 100644
> --- i/generator/API.ml
> +++ w/generator/API.ml
> @@ -784,14 +784,19 @@ "set_pread_initialize", {
> shortdesc = "control whether libnbd pre-initializes read buffers";
> longdesc = "\
> By default, libnbd will pre-initialize the contents of a buffer
> -passed to calls such as L to all zeroes prior to checking
> -for any other errors, so that even if a client application passed in an
> -uninitialized buffer but fails to check for errors, it will not result
> -in a potential security risk caused by an accidental leak of prior heap
> -contents. However, for a client application that has audited that an
> -uninitialized buffer is never dereferenced, or which performs its own
> -pre-initialization, libnbd's sanitization efforts merely pessimize
> -performance.
> +passed to calls such as L to all zeroes prior to
> +checking for any other errors, so that even if a client application
> +passed in an uninitialized buffer but fails to check for errors, it
> +will not result in a potential security risk caused by an accidental
> +leak of prior heap contents (see CVE-2022-0485 in
> +L for an example of a security hole in an
> +application built against an earlier version of libnbd that lacked
> +consistent pre-initialization). However, for a client application
> +that has audited that an uninitialized buffer is never dereferenced,
> +or which performs its own pre-initialization, libnbd's sanitization
> +efforts merely pessimize performance (although the time spent in
> +pre-initialization may pale in comparison to time spent waiting on
> +network packets).
>
> Calling this function with C set to false tells libnbd to
> skip the buffer initialization step in read commands.";
>
>
> >
> > Anyway the whole patch series looks good, so:
> >
> > Reviewed-by: Richard W.M. Jones
>
> Series is now checked in with amendments, as ab52914b..e0953cb7
Thanks.
For Fedora, I won't do anything now, just let this go out in the next
release. It'll need a small adjustment if we backport it to the
stable-1.10 branch.
For RHEL, I'm going to put this in RHEL 9.0, but won't bother with
RHEL 8 (it has ancient history libnbd 1.6).
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH 3/3] api: Add new API nbd_set_pread_initialize()
On Thu, Feb 10, 2022 at 09:38:30AM +, Richard W.M. Jones wrote:
> On Wed, Feb 09, 2022 at 04:07:26PM -0600, Eric Blake wrote:
> > + "set_pread_initialize", {
> > +default_call with
> > +args = [Bool "request"]; ret = RErr;
> > +shortdesc = "control whether libnbd pre-initializes read buffers";
> > +longdesc = "\
> > +By default, libnbd will pre-initialize the contents of a buffer
> > +passed to calls such as L to all zeroes prior to checking
> > +for any other errors, so that even if a client application passed in an
> > +uninitialized buffer but fails to check for errors, it will not result
> > +in a potential security risk caused by an accidental leak of prior heap
> > +contents. However, for a client application that has audited that an
> > +uninitialized buffer is never dereferenced, or which performs its own
> > +pre-initialization, libnbd's sanitization efforts merely pessimize
> > +performance.
> > +
> > +Calling this function with C set to false tells libnbd to
> > +skip the buffer initialization step in read commands.";
> > +see_also = [Link "get_pread_initialize";
> > +Link "set_strict_mode";
> > +Link "pread"; Link "pread_structured"; Link "aio_pread";
> > +Link "aio_pread_structured"];
> > + };
>
> Could it be worth mentioning CVE-2022-0485 by name in the text here?
> And/or linking to:
> https://listman.redhat.com/archives/libguestfs/2022-February/msg00104.html
Good idea, I went with:
diff --git i/generator/API.ml w/generator/API.ml
index 9b7eb545..aaba9951 100644
--- i/generator/API.ml
+++ w/generator/API.ml
@@ -784,14 +784,19 @@ "set_pread_initialize", {
shortdesc = "control whether libnbd pre-initializes read buffers";
longdesc = "\
By default, libnbd will pre-initialize the contents of a buffer
-passed to calls such as L to all zeroes prior to checking
-for any other errors, so that even if a client application passed in an
-uninitialized buffer but fails to check for errors, it will not result
-in a potential security risk caused by an accidental leak of prior heap
-contents. However, for a client application that has audited that an
-uninitialized buffer is never dereferenced, or which performs its own
-pre-initialization, libnbd's sanitization efforts merely pessimize
-performance.
+passed to calls such as L to all zeroes prior to
+checking for any other errors, so that even if a client application
+passed in an uninitialized buffer but fails to check for errors, it
+will not result in a potential security risk caused by an accidental
+leak of prior heap contents (see CVE-2022-0485 in
+L for an example of a security hole in an
+application built against an earlier version of libnbd that lacked
+consistent pre-initialization). However, for a client application
+that has audited that an uninitialized buffer is never dereferenced,
+or which performs its own pre-initialization, libnbd's sanitization
+efforts merely pessimize performance (although the time spent in
+pre-initialization may pale in comparison to time spent waiting on
+network packets).
Calling this function with C set to false tells libnbd to
skip the buffer initialization step in read commands.";
>
> Anyway the whole patch series looks good, so:
>
> Reviewed-by: Richard W.M. Jones
Series is now checked in with amendments, as ab52914b..e0953cb7
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH 2/3] api: Guarantee sanitized buffer on pread failure
On Thu, Feb 10, 2022 at 02:51:56PM +0100, Laszlo Ersek wrote:
> On 02/09/22 23:07, Eric Blake wrote:
> > As mentioned in the previous patch, we left the state of the buffer
> > undefined if we fail pread prior to attempting NBD_CMD_READ. Better
> > is to tweak the generator to sanitize the buffer unconditionally, as a
> > way of hardening against potential bugs in client applications that
> > fail to check for errors, but which should not be leaking
> > uninitialized data. In the process, we can document that the contents
> > of buf are now merely unspecified, rather than undefined (valgrind
> > will no longer complain if you read buf, regardless of how nbd_pread
> > failed).
> >
> > As always calling memset() can be a performance hit if the client also
> > sanitizes the buffer or is willing to take the audit risk, the next
> > patch will add a knob that allows the client to control when this
> > happens.
> > ---
> >
> > +(* Sanitize read buffers before any error is possible. *)
> > +List.iter (
> > + function
> > + | BytesOut (n, count)
> > + | BytesPersistOut (n, count) ->
> > + pr " memset (%s, 0, %s);\n" n count
> > + | _ -> ()
> > +) args;
> > +
> > (* Check current state is permitted for this call. *)
> > if permitted_states <> [] then (
> >let value = match errcode with
>
> It tends to assist reviewers if you include a diff in the cover letter
> (or better yet, in the Notes section of the patch, suitably
> quoted/escaped) that shows the effect of the patch on the generated C
> source code.
Good idea. I'm adding the following to the commit message:
This patch causes changes to just the four APIs
nbd_{aio_,}pread{,_structured}, with the generated lib/api.c changes
looking like:
| --- lib/api.c.bak 2022-02-10 08:15:05.418371357 -0600
| +++ lib/api.c 2022-02-10 08:15:13.224372023 -0600
| @@ -2871,6 +2871,7 @@ nbd_pread (struct nbd_handle *h, void *b
| debug (h, "enter: buf= count=%zu offset=%" PRIu64 "
flags=0x%x", count, offset, flags);
|}
|
| + memset (buf, 0, count);
|if (unlikely (!pread_in_permitted_state (h))) {
| ret = -1;
| goto out;
and similar in patch 3/3, which does
- memset (buf, 0, count);
+ if (h->pread_initialize)
+memset (buf, 0, count);
>
> That said:
>
> Acked-by: Laszlo Ersek
>
> Thanks,
> Laszlo
Thanks for the careful reviews.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH 3/3] api: Add new API nbd_set_pread_initialize()
On 02/09/22 23:07, Eric Blake wrote:
> The recent patch series for CVE-2022-0485 demonstrated that when
> applications using libnbd are not careful about error checking, the
> difference on whether a data leak is at least sanitized (all zeroes,
> partial reads, or data leftover from a prior read) vs. a dangerous
> information leak (uninitialized data from the heap) was partly under
> libnbd's control. The previous two patches changed libnbd to always
> sanitize, as a security hardening technique that prevents heap leaks
> no matter how buggy the client app is. But a blind memset() also adds
> an execution delay, even if it doesn't show up as the hot spot in our
> profiling to the slower time spent with network traffic.
>
> At any rate, if client apps choose to pre-initialize their buffers, or
> otherwise audit their code to take on their own risk about not
> dereferencing a buffer on failure paths, then the time spent by libnbd
> doing memset() is wasted; so it is worth adding a knob to let a user
> opt in to faster execution at the expense of giving up our memset()
> hardening on their behalf.
> ---
> lib/internal.h | 5 +-
> generator/API.ml | 79 +++---
> generator/C.ml | 3 +-
> lib/handle.c | 17 -
> python/t/110-defaults.py | 3 +-
> python/t/120-set-non-defaults.py | 4 +-
> ocaml/tests/test_110_defaults.ml | 4 +-
> ocaml/tests/test_120_set_non_defaults.ml | 5 +-
> tests/errors.c | 25 ++-
> golang/libnbd_110_defaults_test.go | 10 ++-
> golang/libnbd_120_set_non_defaults_test.go | 12
> 11 files changed, 148 insertions(+), 19 deletions(-)
>
> diff --git a/lib/internal.h b/lib/internal.h
> index 0e205aba..525499a9 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -1,5 +1,5 @@
> /* nbd client library in userspace: internal definitions
> - * Copyright (C) 2013-2020 Red Hat Inc.
> + * Copyright (C) 2013-2022 Red Hat Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -123,6 +123,9 @@ struct nbd_handle {
>/* Full info mode. */
>bool full_info;
>
> + /* Sanitization for pread. */
> + bool pread_initialize;
> +
>/* Global flags from the server. */
>uint16_t gflags;
>
> diff --git a/generator/API.ml b/generator/API.ml
> index 98f90031..9b7eb545 100644
> --- a/generator/API.ml
> +++ b/generator/API.ml
> @@ -778,6 +778,44 @@ "get_handshake_flags", {
> Link "aio_is_created"; Link "aio_is_ready"];
>};
>
> + "set_pread_initialize", {
> +default_call with
> +args = [Bool "request"]; ret = RErr;
> +shortdesc = "control whether libnbd pre-initializes read buffers";
> +longdesc = "\
> +By default, libnbd will pre-initialize the contents of a buffer
> +passed to calls such as L to all zeroes prior to checking
> +for any other errors, so that even if a client application passed in an
> +uninitialized buffer but fails to check for errors, it will not result
> +in a potential security risk caused by an accidental leak of prior heap
> +contents. However, for a client application that has audited that an
> +uninitialized buffer is never dereferenced, or which performs its own
> +pre-initialization, libnbd's sanitization efforts merely pessimize
> +performance.
> +
> +Calling this function with C set to false tells libnbd to
> +skip the buffer initialization step in read commands.";
> +see_also = [Link "get_pread_initialize";
> +Link "set_strict_mode";
> +Link "pread"; Link "pread_structured"; Link "aio_pread";
> +Link "aio_pread_structured"];
> + };
> +
> + "get_pread_initialize", {
> +default_call with
> +args = []; ret = RBool;
> +may_set_error = false;
> +shortdesc = "see whether libnbd pre-initializes read buffers";
> +longdesc = "\
> +Return whether libnbd performs a pre-initialization of a buffer passed
> +to L and similar to all zeroes, as set by
> +L.";
> +see_also = [Link "set_pread_initialize";
> +Link "set_strict_mode";
> +Link "pread"; Link "pread_structured"; Link "aio_pread";
> +Link "aio_pread_structured"];
> + };
> +
>"set_strict_mode", {
> default_call with
> args = [ Flags ("flags", strict_flags) ]; ret = RErr;
> @@ -1831,11 +1869,16 @@ "pread", {
> The C parameter must be C<0> for now (it exists for future NBD
> protocol extensions).
>
> -Note that if this command fails, it is unspecified whether the contents
> -of C will read as zero or as partial results from the server."
> +Note that if this command fails, and L
> +returns true, then libnbd sanitized C, but it is unspecified
> +whether the contents of C will read as zero or as partial results
> +from the server. If
Re: [Libguestfs] [libnbd PATCH 2/3] api: Guarantee sanitized buffer on pread failure
On 02/09/22 23:07, Eric Blake wrote:
> As mentioned in the previous patch, we left the state of the buffer
> undefined if we fail pread prior to attempting NBD_CMD_READ. Better
> is to tweak the generator to sanitize the buffer unconditionally, as a
> way of hardening against potential bugs in client applications that
> fail to check for errors, but which should not be leaking
> uninitialized data. In the process, we can document that the contents
> of buf are now merely unspecified, rather than undefined (valgrind
> will no longer complain if you read buf, regardless of how nbd_pread
> failed).
>
> As always calling memset() can be a performance hit if the client also
> sanitizes the buffer or is willing to take the audit risk, the next
> patch will add a knob that allows the client to control when this
> happens.
> ---
> generator/API.ml | 24 ++--
> generator/C.ml | 11 ++-
> lib/rw.c | 18 +++---
> tests/errors.c | 9 -
> 4 files changed, 39 insertions(+), 23 deletions(-)
>
> diff --git a/generator/API.ml b/generator/API.ml
> index 012016bc..98f90031 100644
> --- a/generator/API.ml
> +++ b/generator/API.ml
> @@ -1831,7 +1831,8 @@ "pread", {
> The C parameter must be C<0> for now (it exists for future NBD
> protocol extensions).
>
> -Note that if this command fails, the contents of C are undefined."
> +Note that if this command fails, it is unspecified whether the contents
> +of C will read as zero or as partial results from the server."
> ^ strict_call_description;
> see_also = [Link "aio_pread"; Link "pread_structured";
> Link "get_block_size"; Link "set_strict_mode"];
> @@ -1918,7 +1919,8 @@ "pread_structured", {
> this, see L). Libnbd does not validate that the server
> actually obeys the flag.
>
> -Note that if this command fails, the contents of C are undefined."
> +Note that if this command fails, it is unspecified whether the contents
> +of C will read as zero or as partial results from the server."
> ^ strict_call_description;
> see_also = [Link "can_df"; Link "pread";
> Link "aio_pread_structured"; Link "get_block_size";
> @@ -2459,10 +2461,11 @@ "aio_pread", {
> as described in L.
>
> Note that you must ensure C is valid until the command has
> -completed. Furthermore, the contents of C are undefined if the
> -C parameter to C is set, or if
> -L reports failure. Other parameters behave
> -as documented in L."
> +completed. Furthermore, if the C parameter to
> +C is set or if L
> +reports failure, it is unspecified whether the contents
> +of C will read as zero or as partial results from the server.
> +Other parameters behave as documented in L."
> ^ strict_call_description;
> example = Some "examples/aio-connect-read.c";
> see_also = [SectionLink "Issuing asynchronous commands";
> @@ -2487,10 +2490,11 @@ "aio_pread_structured", {
> as described in L.
>
> Note that you must ensure C is valid until the command has
> -completed. Furthermore, the contents of C are undefined if the
> -C parameter to C is set, or if
> -L reports failure. Other parameters behave
> -as documented in L."
> +completed. Furthermore, if the C parameter to
> +C is set or if L
> +reports failure, it is unspecified whether the contents
> +of C will read as zero or as partial results from the server.
> +Other parameters behave as documented in L."
> ^ strict_call_description;
> see_also = [SectionLink "Issuing asynchronous commands";
> Link "aio_pread"; Link "pread_structured";
> diff --git a/generator/C.ml b/generator/C.ml
> index 797af531..4a5bb589 100644
> --- a/generator/C.ml
> +++ b/generator/C.ml
> @@ -1,6 +1,6 @@
> (* hey emacs, this is OCaml code: -*- tuareg -*- *)
> (* nbd client library in userspace: generate the C API and documentation
> - * Copyright (C) 2013-2020 Red Hat Inc.
> + * Copyright (C) 2013-2022 Red Hat Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -491,6 +491,15 @@ let
>pr "\n"
> );
>
> +(* Sanitize read buffers before any error is possible. *)
> +List.iter (
> + function
> + | BytesOut (n, count)
> + | BytesPersistOut (n, count) ->
> + pr " memset (%s, 0, %s);\n" n count
> + | _ -> ()
> +) args;
> +
> (* Check current state is permitted for this call. *)
> if permitted_states <> [] then (
>let value = match errcode with
It tends to assist reviewers if you include a diff in the cover letter
(or better yet, in the Notes section of the patch, suitably
quoted/escaped) that shows the effect of the patch on the generated C
source code.
That said:
Acked-by: Laszlo Ersek
Thanks,
Laszlo
> diff --git a/lib/rw.c b/lib/rw.c
> index b5d3dc44..1202d71c 100644
> --- a/lib/rw.c
> +++ b/lib/rw.c
> @@ -244,18 +244,14 @@ nbd_internal_command_common (struct nbd_handle *h
Re: [Libguestfs] [libnbd PATCH 1/3] api: Drop server control of memset() prior to NBD_CMD_READ
On 02/09/22 23:07, Eric Blake wrote:
> The recent CVE-2022-0485 demonstrated that clients that pass in an
> uninitialized buffer to nbd_pread and friends, but are then not
> careful about checking for read errors, were subjected to
> server-dependent behavior on whether their use of the buffer after
> failure saw sanitized zeroes or prior heap contents.
>
> Making our choice of whether to sanitize the buffer be under the
> control of what the server negotiates is not ideal. We commonly test
> with servers that support structured replies (nbdkit by default, as
> well as qemu-nbd), and less frequently with servers that lack them
> (nbd-server as of the current nbd-3.23 release, or 'nbdkit --no-sr'),
> thus, we are already used to the runtime speed of libnbd sanitizing a
> read buffer, and more likely to miss the additional security impact
> caused when a less-common server can turn a client bug into a heap
> leak.
>
> This patch makes the sanitization unconditional if we actually reach
> the point of issuing NBD_CMD_READ (to make it easier to backport in
> isolation for distros that want secure-by-default for clients that
> call nbd_pread correctly). Note that with just this patch, there are
> still some nbd_pread errors where the buffer is left uninitialized
> (such as calling nbd_pread before nbd_connect, or if the pread is
> aborted client-side due to detecting invalid parameters with
> nbd_set_strict_mode); but client apps are generally less likely to
> trip those corner cases, and any heap leak in those cases is not
> dependent on server behavior.
>
> Then upcoming patches will then hoist the memset() even earlier, to
> give guaranteed buffer contents on all error paths, then add a new API
> to allow clients to inform libnbd when to skip the sanitization as an
> optimization (either because the client sanitized the buffer itself,
> or has audited to ensure that an uninitialized buffer is not
> dereferenced).
>
> Fixes: 7c2543e9 ("lib: For structured replies, zero the buffer ahead of
> time.", v0.1)
> ---
> lib/rw.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/lib/rw.c b/lib/rw.c
> index 4ade7508..b5d3dc44 100644
> --- a/lib/rw.c
> +++ b/lib/rw.c
> @@ -1,5 +1,5 @@
> /* NBD client library in userspace
> - * Copyright (C) 2013-2020 Red Hat Inc.
> + * Copyright (C) 2013-2020, 2022 Red Hat Inc.
> *
> * This library is free software; you can redistribute it and/or
> * modify it under the terms of the GNU Lesser General Public
> @@ -250,9 +250,11 @@ nbd_internal_command_common (struct nbd_handle *h,
> * ahead of time which avoids any security problems. I measured the
> * overhead of this and for non-TLS there is no measurable overhead
> * in the highly intensive loopback case. For TLS we get a
> - * performance gain, go figure.
> + * performance gain, go figure. For an older server with only
> + * simple replies, it's still better to do the same memset() so we
> + * don't have behavior that is server-dependent.
> */
> - if (h->structured_replies && cmd->data && type == NBD_CMD_READ)
> + if (cmd->data && type == NBD_CMD_READ)
> memset (cmd->data, 0, cmd->count);
>
>/* Add the command to the end of the queue. Kick the state machine
>
Acked-by: Laszlo Ersek
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [guestfs-tools PATCH] virt-resize: replace "wrap" calls with calls to "info"
On 02/09/22 14:36, Richard W.M. Jones wrote: > On Wed, Feb 09, 2022 at 02:17:38PM +0100, Laszlo Ersek wrote: >> Utilities shouldn't directly call "Std_utils.wrap" for printing >> informative messages; the "Tools_utils.info" function handles that better. >> >> Because "info" prints a trailing newline automatically, strip one newline >> from the originally wrapped messages. While at it, rewrap (in the source >> code) the "Resize operation completed with no errors" message, for better >> readability. >> >> The patch introduces some visible (but, arguably, correct) changes to the >> output: >> >>> virt-resize: /dev/sda1: This partition will be resized from 1023.9M to >>> 2.0G. The filesystem ext4 on /dev/sda1 will be expanded using the >>> ‘resize2fs’ method. >>> [...] >>> virt-resize: Resize operation completed with no errors. Before deleting >>> the old disk, carefully check that the resized disk boots and works >>> correctly. >> >> These messages now carry the "virt-resize: " prefix, and they are printed >> in magenta. >> >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1820221 >> Signed-off-by: Laszlo Ersek >> --- >> resize/resize.ml | 10 ++ >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/resize/resize.ml b/resize/resize.ml >> index dad453ff99b7..b77e680d49c8 100644 >> --- a/resize/resize.ml >> +++ b/resize/resize.ml >> @@ -938,7 +938,7 @@ read the man page virt-resize(1). >>"" >> ) in >> >> - wrap (text ^ "\n\n") in >> + info "%s" (text ^ "\n") in >> >> List.iter print_summary partitions; >> >> @@ -969,7 +969,7 @@ read the man page virt-resize(1). >> "" >>) in >> >> -wrap (text ^ "\n\n") >> +info "%s" (text ^ "\n") >> ) lvs; >> >> if surplus > 0L then ( >> @@ -983,7 +983,7 @@ read the man page virt-resize(1). >> ) else >>s_" The surplus space will be ignored. Run a partitioning >> program in the guest to partition this extra space if you want." in >> >> - wrap (text ^ "\n\n") >> + info "%s" (text ^ "\n") >> ); >> >> printf "**\n"; >> @@ -1440,7 +1440,9 @@ read the man page virt-resize(1). >> >>if not (quiet ()) then ( >> print_newline (); >> -wrap (s_"Resize operation completed with no errors. Before deleting >> the old disk, carefully check that the resized disk boots and works >> correctly.\n"); >> +info "%s" (s_"Resize operation completed with no errors. Before >> deleting \ >> + the old disk, carefully check that the resized disk boots >> and \ >> + works correctly."); >>) >> >> let () = run_main_and_handle_errors main >> >> base-commit: 7d5d5e921d3d483a997f40566c1ccabf8a689a8a >> -- >> 2.19.1.3.g30247aa5d201 > > Yes this is fine, ACK. Thanks, merged as commit 626f0441d251. > Should we also hide the wrap function? From a very quick look at the > code it seems we could move the function from mlstdutils/std_utils.ml > to mltools/tools_utils.ml, and of course drop the public interface in > mlstdutils/std_utils.mli. Yes, good idea; I'll try to do that at the front of my upcoming patch series for libguestfs-common. Thanks! Laszlo ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [libnbd PATCH 3/3] api: Add new API nbd_set_pread_initialize()
On Wed, Feb 09, 2022 at 04:07:26PM -0600, Eric Blake wrote:
> + "set_pread_initialize", {
> +default_call with
> +args = [Bool "request"]; ret = RErr;
> +shortdesc = "control whether libnbd pre-initializes read buffers";
> +longdesc = "\
> +By default, libnbd will pre-initialize the contents of a buffer
> +passed to calls such as L to all zeroes prior to checking
> +for any other errors, so that even if a client application passed in an
> +uninitialized buffer but fails to check for errors, it will not result
> +in a potential security risk caused by an accidental leak of prior heap
> +contents. However, for a client application that has audited that an
> +uninitialized buffer is never dereferenced, or which performs its own
> +pre-initialization, libnbd's sanitization efforts merely pessimize
> +performance.
> +
> +Calling this function with C set to false tells libnbd to
> +skip the buffer initialization step in read commands.";
> +see_also = [Link "get_pread_initialize";
> +Link "set_strict_mode";
> +Link "pread"; Link "pread_structured"; Link "aio_pread";
> +Link "aio_pread_structured"];
> + };
Could it be worth mentioning CVE-2022-0485 by name in the text here?
And/or linking to:
https://listman.redhat.com/archives/libguestfs/2022-February/msg00104.html
Anyway the whole patch series looks good, so:
Reviewed-by: Richard W.M. Jones
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
