[PATCH v3 0/3] hw/virtio: Minor housekeeping patches

[PATCH v3 0/3] hw/virtio: Minor housekeeping patches

[Philippe Mathieu-Daudé]

Hi,

This series contains few patches I gathered while tooking notes
trying to understand issues #300-#302.

Since v2:
- Rebased on top of 88afdc92b64 ("Merge 'remotes/mst/tags/for_upstream' into staging")

Since v1:
- Added virtqueue_flush comment (Stefano)
- Call RCU_READ_LOCK_GUARD in virtqueue_packed_drop_all (Stefano)

Philippe Mathieu-Daudé (3):
  hw/virtio: Comment virtqueue_flush() must be called with RCU read lock
  hw/virtio: Acquire RCU read lock in virtqueue_packed_drop_all()
  hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees

 include/hw/virtio/virtio.h |  7 +++++++
 hw/virtio/virtio.c         | 32 +++++++++++++++-----------------
 2 files changed, 22 insertions(+), 17 deletions(-)

-- 
2.31.1

[PATCH v3 1/3] hw/virtio: Comment virtqueue_flush() must be called with RCU read lock

[Philippe Mathieu-Daudé]

Reported-by: Stefano Garzarella <sgarzare@redhat.com>
Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/hw/virtio/virtio.h | 7 +++++++
 hw/virtio/virtio.c         | 1 +
 2 files changed, 8 insertions(+)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 8bab9cfb750..c1c5f6e53c8 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -186,6 +186,13 @@ void virtio_delete_queue(VirtQueue *vq);
 
 void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
                     unsigned int len);
+/**
+ * virtqueue_flush:
+ * @vq: The #VirtQueue
+ * @count: Number of elements to flush
+ *
+ * Must be called within RCU critical section.
+ */
 void virtqueue_flush(VirtQueue *vq, unsigned int count);
 void virtqueue_detach_element(VirtQueue *vq, const VirtQueueElement *elem,
                               unsigned int len);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 3a1f6c520cb..97f60017466 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -896,6 +896,7 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count)
     }
 }
 
+/* Called within rcu_read_lock().  */
 void virtqueue_flush(VirtQueue *vq, unsigned int count)
 {
     if (virtio_device_disabled(vq->vdev)) {
-- 
2.31.1

[PATCH v3 2/3] hw/virtio: Acquire RCU read lock in virtqueue_packed_drop_all()

[Philippe Mathieu-Daudé]

vring_get_region_caches() must be called with the RCU read lock
acquired. virtqueue_packed_drop_all() does not, and uses the
'caches' pointer. Fix that by using the RCU_READ_LOCK_GUARD()
macro.

Reported-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/virtio/virtio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 97f60017466..7d3bf9091ee 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1704,6 +1704,8 @@ static unsigned int virtqueue_packed_drop_all(VirtQueue *vq)
     VirtIODevice *vdev = vq->vdev;
     VRingPackedDesc desc;
 
+    RCU_READ_LOCK_GUARD();
+
     caches = vring_get_region_caches(vq);
     if (!caches) {
         return 0;
-- 
2.31.1

[PATCH v3 3/3] hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees

[Philippe Mathieu-Daudé]

Both virtqueue_packed_get_avail_bytes() and
virtqueue_split_get_avail_bytes() access the region cache, but
their caller also does. Simplify by having virtqueue_get_avail_bytes
calling both with RCU lock held, and passing the caches as argument.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/virtio/virtio.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 7d3bf9091ee..0dbfb53e51b 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -985,28 +985,23 @@ static int virtqueue_split_read_next_desc(VirtIODevice *vdev, VRingDesc *desc,
     return VIRTQUEUE_READ_DESC_MORE;
 }
 
+/* Called within rcu_read_lock().  */
 static void virtqueue_split_get_avail_bytes(VirtQueue *vq,
                             unsigned int *in_bytes, unsigned int *out_bytes,
-                            unsigned max_in_bytes, unsigned max_out_bytes)
+                            unsigned max_in_bytes, unsigned max_out_bytes,
+                            VRingMemoryRegionCaches *caches)
 {
     VirtIODevice *vdev = vq->vdev;
     unsigned int max, idx;
     unsigned int total_bufs, in_total, out_total;
-    VRingMemoryRegionCaches *caches;
     MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
     int64_t len = 0;
     int rc;
 
-    RCU_READ_LOCK_GUARD();
-
     idx = vq->last_avail_idx;
     total_bufs = in_total = out_total = 0;
 
     max = vq->vring.num;
-    caches = vring_get_region_caches(vq);
-    if (!caches) {
-        goto err;
-    }
 
     while ((rc = virtqueue_num_heads(vq, idx)) > 0) {
         MemoryRegionCache *desc_cache = &caches->desc;
@@ -1125,32 +1120,28 @@ static int virtqueue_packed_read_next_desc(VirtQueue *vq,
     return VIRTQUEUE_READ_DESC_MORE;
 }
 
+/* Called within rcu_read_lock().  */
 static void virtqueue_packed_get_avail_bytes(VirtQueue *vq,
                                              unsigned int *in_bytes,
                                              unsigned int *out_bytes,
                                              unsigned max_in_bytes,
-                                             unsigned max_out_bytes)
+                                             unsigned max_out_bytes,
+                                             VRingMemoryRegionCaches *caches)
 {
     VirtIODevice *vdev = vq->vdev;
     unsigned int max, idx;
     unsigned int total_bufs, in_total, out_total;
     MemoryRegionCache *desc_cache;
-    VRingMemoryRegionCaches *caches;
     MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
     int64_t len = 0;
     VRingPackedDesc desc;
     bool wrap_counter;
 
-    RCU_READ_LOCK_GUARD();
     idx = vq->last_avail_idx;
     wrap_counter = vq->last_avail_wrap_counter;
     total_bufs = in_total = out_total = 0;
 
     max = vq->vring.num;
-    caches = vring_get_region_caches(vq);
-    if (!caches) {
-        goto err;
-    }
 
     for (;;) {
         unsigned int num_bufs = total_bufs;
@@ -1251,6 +1242,8 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
     uint16_t desc_size;
     VRingMemoryRegionCaches *caches;
 
+    RCU_READ_LOCK_GUARD();
+
     if (unlikely(!vq->vring.desc)) {
         goto err;
     }
@@ -1269,10 +1262,12 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
 
     if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
         virtqueue_packed_get_avail_bytes(vq, in_bytes, out_bytes,
-                                         max_in_bytes, max_out_bytes);
+                                         max_in_bytes, max_out_bytes,
+                                         caches);
     } else {
         virtqueue_split_get_avail_bytes(vq, in_bytes, out_bytes,
-                                        max_in_bytes, max_out_bytes);
+                                        max_in_bytes, max_out_bytes,
+                                        caches);
     }
 
     return;
-- 
2.31.1

Re: [PATCH v3 0/3] hw/virtio: Minor housekeeping patches

[Stefano Garzarella]

On Mon, Sep 06, 2021 at 12:43:15PM +0200, Philippe Mathieu-Daudé wrote:
>Hi,
>
>This series contains few patches I gathered while tooking notes
>trying to understand issues #300-#302.
>
>Since v2:
>- Rebased on top of 88afdc92b64 ("Merge 'remotes/mst/tags/for_upstream' into staging")
>
>Since v1:
>- Added virtqueue_flush comment (Stefano)
>- Call RCU_READ_LOCK_GUARD in virtqueue_packed_drop_all (Stefano)
>
>Philippe Mathieu-Daudé (3):
>  hw/virtio: Comment virtqueue_flush() must be called with RCU read lock
>  hw/virtio: Acquire RCU read lock in virtqueue_packed_drop_all()
>  hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees
>
> include/hw/virtio/virtio.h |  7 +++++++
> hw/virtio/virtio.c         | 32 +++++++++++++++-----------------
> 2 files changed, 22 insertions(+), 17 deletions(-)
>
>-- 
>2.31.1
>
>

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Re: [PATCH v3 1/3] hw/virtio: Comment virtqueue_flush() must be called with RCU read lock

[Cornelia Huck]

On Mon, Sep 06 2021, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Reported-by: Stefano Garzarella <sgarzare@redhat.com>
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/hw/virtio/virtio.h | 7 +++++++
>  hw/virtio/virtio.c         | 1 +
>  2 files changed, 8 insertions(+)
>
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 8bab9cfb750..c1c5f6e53c8 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -186,6 +186,13 @@ void virtio_delete_queue(VirtQueue *vq);
>  
>  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>                      unsigned int len);
> +/**
> + * virtqueue_flush:
> + * @vq: The #VirtQueue
> + * @count: Number of elements to flush
> + *
> + * Must be called within RCU critical section.
> + */

Hm... do these doc comments belong into .h files, or rather into .c files?

>  void virtqueue_flush(VirtQueue *vq, unsigned int count);
>  void virtqueue_detach_element(VirtQueue *vq, const VirtQueueElement *elem,
>                                unsigned int len);
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 3a1f6c520cb..97f60017466 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -896,6 +896,7 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count)
>      }
>  }
>  
> +/* Called within rcu_read_lock().  */
>  void virtqueue_flush(VirtQueue *vq, unsigned int count)
>  {
>      if (virtio_device_disabled(vq->vdev)) {

The content of the change looks good to me, I'm only wondering about
the style...

Re: [PATCH v3 1/3] hw/virtio: Comment virtqueue_flush() must be called with RCU read lock

[Philippe Mathieu-Daudé]

On 9/27/21 12:18, Cornelia Huck wrote:
> On Mon, Sep 06 2021, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> Reported-by: Stefano Garzarella <sgarzare@redhat.com>
>> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  include/hw/virtio/virtio.h | 7 +++++++
>>  hw/virtio/virtio.c         | 1 +
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index 8bab9cfb750..c1c5f6e53c8 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -186,6 +186,13 @@ void virtio_delete_queue(VirtQueue *vq);
>>  
>>  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>>                      unsigned int len);
>> +/**
>> + * virtqueue_flush:
>> + * @vq: The #VirtQueue
>> + * @count: Number of elements to flush
>> + *
>> + * Must be called within RCU critical section.
>> + */
> 
> Hm... do these doc comments belong into .h files, or rather into .c files?

Maybe we should restart this old thread, vote(?) and settle on a style?

https://lore.kernel.org/qemu-devel/349cd87b-0526-30b8-d9cd-0eee537ab5a4@redhat.com/

>>  void virtqueue_flush(VirtQueue *vq, unsigned int count);
>>  void virtqueue_detach_element(VirtQueue *vq, const VirtQueueElement *elem,
>>                                unsigned int len);
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 3a1f6c520cb..97f60017466 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -896,6 +896,7 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count)
>>      }
>>  }
>>  
>> +/* Called within rcu_read_lock().  */
>>  void virtqueue_flush(VirtQueue *vq, unsigned int count)
>>  {
>>      if (virtio_device_disabled(vq->vdev)) {
> 
> The content of the change looks good to me, I'm only wondering about
> the style...
> 

Re: [PATCH v3 1/3] hw/virtio: Comment virtqueue_flush() must be called with RCU read lock

[Cornelia Huck]

On Mon, Sep 27 2021, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 9/27/21 12:18, Cornelia Huck wrote:
>> On Mon, Sep 06 2021, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> 
>>> Reported-by: Stefano Garzarella <sgarzare@redhat.com>
>>> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>  include/hw/virtio/virtio.h | 7 +++++++
>>>  hw/virtio/virtio.c         | 1 +
>>>  2 files changed, 8 insertions(+)
>>>
>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>>> index 8bab9cfb750..c1c5f6e53c8 100644
>>> --- a/include/hw/virtio/virtio.h
>>> +++ b/include/hw/virtio/virtio.h
>>> @@ -186,6 +186,13 @@ void virtio_delete_queue(VirtQueue *vq);
>>>  
>>>  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>>>                      unsigned int len);
>>> +/**
>>> + * virtqueue_flush:
>>> + * @vq: The #VirtQueue
>>> + * @count: Number of elements to flush
>>> + *
>>> + * Must be called within RCU critical section.
>>> + */
>> 
>> Hm... do these doc comments belong into .h files, or rather into .c files?
>
> Maybe we should restart this old thread, vote(?) and settle on a style?
>
> https://lore.kernel.org/qemu-devel/349cd87b-0526-30b8-d9cd-0eee537ab5a4@redhat.com/

My vote would still go to putting this into .c files :) Not sure if we
want to go through the hassle of a wholesale cleanup; but if others
agree, we could at least start with putting new doc comments next to the
implementation.

Do we actually consume these comments in an automated way somewhere?

Re: [PATCH v3 1/3] hw/virtio: Comment virtqueue_flush() must be called with RCU read lock

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1969 bytes --]

On Mon, Sep 27, 2021 at 01:29:46PM +0200, Cornelia Huck wrote:
> On Mon, Sep 27 2021, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
> > On 9/27/21 12:18, Cornelia Huck wrote:
> >> On Mon, Sep 06 2021, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >> 
> >>> Reported-by: Stefano Garzarella <sgarzare@redhat.com>
> >>> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >>> ---
> >>>  include/hw/virtio/virtio.h | 7 +++++++
> >>>  hw/virtio/virtio.c         | 1 +
> >>>  2 files changed, 8 insertions(+)
> >>>
> >>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> >>> index 8bab9cfb750..c1c5f6e53c8 100644
> >>> --- a/include/hw/virtio/virtio.h
> >>> +++ b/include/hw/virtio/virtio.h
> >>> @@ -186,6 +186,13 @@ void virtio_delete_queue(VirtQueue *vq);
> >>>  
> >>>  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
> >>>                      unsigned int len);
> >>> +/**
> >>> + * virtqueue_flush:
> >>> + * @vq: The #VirtQueue
> >>> + * @count: Number of elements to flush
> >>> + *
> >>> + * Must be called within RCU critical section.
> >>> + */
> >> 
> >> Hm... do these doc comments belong into .h files, or rather into .c files?
> >
> > Maybe we should restart this old thread, vote(?) and settle on a style?
> >
> > https://lore.kernel.org/qemu-devel/349cd87b-0526-30b8-d9cd-0eee537ab5a4@redhat.com/
> 
> My vote would still go to putting this into .c files :) Not sure if we
> want to go through the hassle of a wholesale cleanup; but if others
> agree, we could at least start with putting new doc comments next to the
> implementation.

In the virtio.c/h case doc comments (and especially the RCU-related
ones) are in the .c file. The exception is that constants and structs
are documented in the header file.

I would follow that and avoid duplicating doc comments into the .h file.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/3] hw/virtio: Acquire RCU read lock in virtqueue_packed_drop_all()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1110 bytes --]

On Mon, Sep 06, 2021 at 12:43:17PM +0200, Philippe Mathieu-Daudé wrote:
> vring_get_region_caches() must be called with the RCU read lock
> acquired. virtqueue_packed_drop_all() does not, and uses the
> 'caches' pointer. Fix that by using the RCU_READ_LOCK_GUARD()
> macro.

Is this a bug that has been encountered, is it a latent bug, a code
cleanup, etc? The impact of this isn't clear but it sounds a little
scary so I wanted to check.

> 
> Reported-by: Stefano Garzarella <sgarzare@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/virtio/virtio.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 97f60017466..7d3bf9091ee 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1704,6 +1704,8 @@ static unsigned int virtqueue_packed_drop_all(VirtQueue *vq)
>      VirtIODevice *vdev = vq->vdev;
>      VRingPackedDesc desc;
>  
> +    RCU_READ_LOCK_GUARD();
> +
>      caches = vring_get_region_caches(vq);
>      if (!caches) {
>          return 0;
> -- 
> 2.31.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 3/3] hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 568 bytes --]

On Mon, Sep 06, 2021 at 12:43:18PM +0200, Philippe Mathieu-Daudé wrote:
> Both virtqueue_packed_get_avail_bytes() and
> virtqueue_split_get_avail_bytes() access the region cache, but
> their caller also does. Simplify by having virtqueue_get_avail_bytes
> calling both with RCU lock held, and passing the caches as argument.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/virtio/virtio.c | 29 ++++++++++++-----------------
>  1 file changed, 12 insertions(+), 17 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/3] hw/virtio: Acquire RCU read lock in virtqueue_packed_drop_all()

[Philippe Mathieu-Daudé]

On 10/4/21 11:23, Stefan Hajnoczi wrote:
> On Mon, Sep 06, 2021 at 12:43:17PM +0200, Philippe Mathieu-Daudé wrote:
>> vring_get_region_caches() must be called with the RCU read lock
>> acquired. virtqueue_packed_drop_all() does not, and uses the
>> 'caches' pointer. Fix that by using the RCU_READ_LOCK_GUARD()
>> macro.
> 
> Is this a bug that has been encountered, is it a latent bug, a code
> cleanup, etc? The impact of this isn't clear but it sounds a little
> scary so I wanted to check.

I'll defer to Stefano, but IIUC it is a latent bug discovered
during code audit.

> 
>>
>> Reported-by: Stefano Garzarella <sgarzare@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  hw/virtio/virtio.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 97f60017466..7d3bf9091ee 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -1704,6 +1704,8 @@ static unsigned int virtqueue_packed_drop_all(VirtQueue *vq)
>>      VirtIODevice *vdev = vq->vdev;
>>      VRingPackedDesc desc;
>>  
>> +    RCU_READ_LOCK_GUARD();
>> +
>>      caches = vring_get_region_caches(vq);
>>      if (!caches) {
>>          return 0;
>> -- 
>> 2.31.1
>>

Re: [PATCH v3 2/3] hw/virtio: Acquire RCU read lock in virtqueue_packed_drop_all()

[Stefano Garzarella]

On Mon, Oct 04, 2021 at 11:27:12AM +0200, Philippe Mathieu-Daudé wrote:
>On 10/4/21 11:23, Stefan Hajnoczi wrote:
>> On Mon, Sep 06, 2021 at 12:43:17PM +0200, Philippe Mathieu-Daudé wrote:
>>> vring_get_region_caches() must be called with the RCU read lock
>>> acquired. virtqueue_packed_drop_all() does not, and uses the
>>> 'caches' pointer. Fix that by using the RCU_READ_LOCK_GUARD()
>>> macro.
>>
>> Is this a bug that has been encountered, is it a latent bug, a code
>> cleanup, etc? The impact of this isn't clear but it sounds a little
>> scary so I wanted to check.
>
>I'll defer to Stefano, but IIUC it is a latent bug discovered
>during code audit.

Yep, I confirm this. We discovered it by discussing the documentation in 
a previous series.

Thanks,
Stefano