[Xen-devel] [PATCH 1/2] argo: warn sendv() caller when ring is full

[Xen-devel] [PATCH 1/2] argo: warn sendv() caller when ring is full

[Nicholas Tsirakis]

In its current state, if the destination ring is full, sendv()
will requeue the message and return the rc of pending_requeue(),
which will return 0 on success. This prevents the caller from
distinguishing the difference between a successful write and a
message that needs to be resent at a later time.

Instead, capture the -EAGAIN value returned from ringbuf_insert()
and *only* overwrite it if the rc of pending_requeue() is non-zero.
This allows the caller to make intelligent decisions on -EAGAIN and
still be alerted if the pending message fails to requeue.

Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com>
---
 xen/common/argo.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/common/argo.c b/xen/common/argo.c
index 13052b9239..2f874a570d 100644
--- a/xen/common/argo.c
+++ b/xen/common/argo.c
@@ -2048,9 +2048,13 @@ sendv(struct domain *src_d, xen_argo_addr_t *src_addr,
                              message_type, &len);
         if ( ret == -EAGAIN )
         {
+            int rc;
+
             argo_dprintk("argo_ringbuf_sendv failed, EAGAIN\n");
             /* requeue to issue a notification when space is there */
-            ret = pending_requeue(dst_d, ring_info, src_id.domain_id, len);
+            rc = pending_requeue(dst_d, ring_info, src_id.domain_id, len);
+            if ( rc )
+                ret = rc;
         }
 
         spin_unlock(&ring_info->L3_lock);
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/2] argo: correctly report pending message length

[Nicholas Tsirakis]

When a message is requeue'd in Xen's internal queue, the queue
entry contains the length of the message so that Xen knows to
send a VIRQ to the respective domain when enough space frees up
in the ring. Due to a small bug, however, Xen doesn't populate
the length of the msg if a given write fails, so this length is
always reported as zero. This causes Xen to spurriously wake up
a domain even when the ring doesn't have enough space.

This patch makes sure that the msg len is properly reported by
populating it in the event of a write failure.

Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com>
---
 xen/common/argo.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/common/argo.c b/xen/common/argo.c
index 2f874a570d..eb541829d6 100644
--- a/xen/common/argo.c
+++ b/xen/common/argo.c
@@ -2050,6 +2050,12 @@ sendv(struct domain *src_d, xen_argo_addr_t *src_addr,
         {
             int rc;
 
+            /*
+             * if ringbuf_insert fails, then len will never be populated.
+             * make sure to populate it here.
+             */
+            iov_count(iovs, niov, &len);
+
             argo_dprintk("argo_ringbuf_sendv failed, EAGAIN\n");
             /* requeue to issue a notification when space is there */
             rc = pending_requeue(dst_d, ring_info, src_id.domain_id, len);
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] argo: warn sendv() caller when ring is full

[Christopher Clark]

On Tue, Jun 11, 2019 at 10:11 AM Nicholas Tsirakis
<niko.tsirakis@gmail.com> wrote:
>
> In its current state, if the destination ring is full, sendv()
> will requeue the message and return the rc of pending_requeue(),
> which will return 0 on success. This prevents the caller from
> distinguishing the difference between a successful write and a
> message that needs to be resent at a later time.
>
> Instead, capture the -EAGAIN value returned from ringbuf_insert()
> and *only* overwrite it if the rc of pending_requeue() is non-zero.
> This allows the caller to make intelligent decisions on -EAGAIN and
> still be alerted if the pending message fails to requeue.
>
> Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com>

Thanks for the correct identification of the problem and the patch.

Reviewed-by: Christopher Clark <christopher.clark6@baesystems.com>


> ---
>  xen/common/argo.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/argo.c b/xen/common/argo.c
> index 13052b9239..2f874a570d 100644
> --- a/xen/common/argo.c
> +++ b/xen/common/argo.c
> @@ -2048,9 +2048,13 @@ sendv(struct domain *src_d, xen_argo_addr_t *src_addr,
>                               message_type, &len);
>          if ( ret == -EAGAIN )
>          {
> +            int rc;
> +
>              argo_dprintk("argo_ringbuf_sendv failed, EAGAIN\n");
>              /* requeue to issue a notification when space is there */
> -            ret = pending_requeue(dst_d, ring_info, src_id.domain_id, len);
> +            rc = pending_requeue(dst_d, ring_info, src_id.domain_id, len);
> +            if ( rc )
> +                ret = rc;
>          }
>
>          spin_unlock(&ring_info->L3_lock);
> --
> 2.17.1
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] argo: correctly report pending message length

[Christopher Clark]

On Tue, Jun 11, 2019 at 10:11 AM Nicholas Tsirakis
<niko.tsirakis@gmail.com> wrote:
>
> When a message is requeue'd in Xen's internal queue, the queue
> entry contains the length of the message so that Xen knows to
> send a VIRQ to the respective domain when enough space frees up
> in the ring. Due to a small bug, however, Xen doesn't populate
> the length of the msg if a given write fails, so this length is
> always reported as zero. This causes Xen to spurriously wake up
> a domain even when the ring doesn't have enough space.
>
> This patch makes sure that the msg len is properly reported by
> populating it in the event of a write failure.

You're correct that this is an issue to be fixed, but unfortunately
this patch doesn't compile, at least with gcc 8.2 with warnings as
errors, reporting:

argo.c: In function 'sendv':
argo.c:2057:35: error: passing argument 3 of 'iov_count' from
incompatible pointer type [-Werror=incompatible-pointer-types]
             iov_count(iovs, niov, &len);
                                   ^~~~
argo.c:723:25: note: expected 'unsigned int *' but argument is of type
'long unsigned int *'
           unsigned int *count)
           ~~~~~~~~~~~~~~^~~~~

Even without this error, the logic it implements can unnecessarily
invoke iov_count twice upon the same guest-supplied buffers; it would
be better to avoid that, so: looking at the original section of code:

* sendv's "len" variable can be int, rather than long.
* iov_count can be called from sendv, just before ringbuf_insert,
instead of within ringbuf_insert. It can populate sendv's "len"
variable.
* the len obtained from iov_count (if successful) can be passed into
ringbuf_insert as a parameter, and replace ringbuf_insert's existing
"len" variable.
* ringbuf_insert's "out_len" pointer argument can then be dropped as
unnecessary.
* pending_requeue will be fine to use sendv's populated "len" variable.

Christopher


> Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com>
> ---
>  xen/common/argo.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/xen/common/argo.c b/xen/common/argo.c
> index 2f874a570d..eb541829d6 100644
> --- a/xen/common/argo.c
> +++ b/xen/common/argo.c
> @@ -2050,6 +2050,12 @@ sendv(struct domain *src_d, xen_argo_addr_t *src_addr,
>          {
>              int rc;
>
> +            /*
> +             * if ringbuf_insert fails, then len will never be populated.
> +             * make sure to populate it here.
> +             */
> +            iov_count(iovs, niov, &len);
> +
>              argo_dprintk("argo_ringbuf_sendv failed, EAGAIN\n");
>              /* requeue to issue a notification when space is there */
>              rc = pending_requeue(dst_d, ring_info, src_id.domain_id, len);
> --
> 2.17.1
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] argo: warn sendv() caller when ring is full

[Andrew Cooper]

On 11/06/2019 19:43, Christopher Clark wrote:
> On Tue, Jun 11, 2019 at 10:11 AM Nicholas Tsirakis
> <niko.tsirakis@gmail.com> wrote:
>> In its current state, if the destination ring is full, sendv()
>> will requeue the message and return the rc of pending_requeue(),
>> which will return 0 on success. This prevents the caller from
>> distinguishing the difference between a successful write and a
>> message that needs to be resent at a later time.
>>
>> Instead, capture the -EAGAIN value returned from ringbuf_insert()
>> and *only* overwrite it if the rc of pending_requeue() is non-zero.
>> This allows the caller to make intelligent decisions on -EAGAIN and
>> still be alerted if the pending message fails to requeue.
>>
>> Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com>
> Thanks for the correct identification of the problem and the patch.
>
> Reviewed-by: Christopher Clark <christopher.clark6@baesystems.com>

So I was coming to commit this, but technically according to the
maintainers file, ARGO is maintained by <christopher.w.clark@gmail.com>

Looking at the ARGO series as committed, the patches where all From:
gmail, SoB: baesystems.

Which is the correct alias to use?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] argo: warn sendv() caller when ring is full

[Christopher Clark]

On Tue, Jun 11, 2019 at 12:16 PM Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>
> On 11/06/2019 19:43, Christopher Clark wrote:
> > On Tue, Jun 11, 2019 at 10:11 AM Nicholas Tsirakis
> > <niko.tsirakis@gmail.com> wrote:
> >> In its current state, if the destination ring is full, sendv()
> >> will requeue the message and return the rc of pending_requeue(),
> >> which will return 0 on success. This prevents the caller from
> >> distinguishing the difference between a successful write and a
> >> message that needs to be resent at a later time.
> >>
> >> Instead, capture the -EAGAIN value returned from ringbuf_insert()
> >> and *only* overwrite it if the rc of pending_requeue() is non-zero.
> >> This allows the caller to make intelligent decisions on -EAGAIN and
> >> still be alerted if the pending message fails to requeue.
> >>
> >> Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com>
> > Thanks for the correct identification of the problem and the patch.
> >
> > Reviewed-by: Christopher Clark <christopher.clark6@baesystems.com>
>
> So I was coming to commit this, but technically according to the
> maintainers file, ARGO is maintained by <christopher.w.clark@gmail.com>
>
> Looking at the ARGO series as committed, the patches where all From:
> gmail, SoB: baesystems.
>
> Which is the correct alias to use?

For this purpose:
Reviewed-by: Christopher Clark <christopher.w.clark@gmail.com>

thanks

Christopher

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] argo: warn sendv() caller when ring is full

[Andrew Cooper]

On 11/06/2019 20:22, Christopher Clark wrote:
> On Tue, Jun 11, 2019 at 12:16 PM Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>> On 11/06/2019 19:43, Christopher Clark wrote:
>>> On Tue, Jun 11, 2019 at 10:11 AM Nicholas Tsirakis
>>> <niko.tsirakis@gmail.com> wrote:
>>>> In its current state, if the destination ring is full, sendv()
>>>> will requeue the message and return the rc of pending_requeue(),
>>>> which will return 0 on success. This prevents the caller from
>>>> distinguishing the difference between a successful write and a
>>>> message that needs to be resent at a later time.
>>>>
>>>> Instead, capture the -EAGAIN value returned from ringbuf_insert()
>>>> and *only* overwrite it if the rc of pending_requeue() is non-zero.
>>>> This allows the caller to make intelligent decisions on -EAGAIN and
>>>> still be alerted if the pending message fails to requeue.
>>>>
>>>> Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com>
>>> Thanks for the correct identification of the problem and the patch.
>>>
>>> Reviewed-by: Christopher Clark <christopher.clark6@baesystems.com>
>> So I was coming to commit this, but technically according to the
>> maintainers file, ARGO is maintained by <christopher.w.clark@gmail.com>
>>
>> Looking at the ARGO series as committed, the patches where all From:
>> gmail, SoB: baesystems.
>>
>> Which is the correct alias to use?
> For this purpose:
> Reviewed-by: Christopher Clark <christopher.w.clark@gmail.com>

Fixed up and pushed.  Thanks.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] argo: correctly report pending message length

[Nicholas Tsirakis]

On Tue, Jun 11, 2019 at 2:49 PM Christopher Clark
<christopher.w.clark@gmail.com> wrote:
>
> On Tue, Jun 11, 2019 at 10:11 AM Nicholas Tsirakis
> <niko.tsirakis@gmail.com> wrote:
> >
> > When a message is requeue'd in Xen's internal queue, the queue
> > entry contains the length of the message so that Xen knows to
> > send a VIRQ to the respective domain when enough space frees up
> > in the ring. Due to a small bug, however, Xen doesn't populate
> > the length of the msg if a given write fails, so this length is
> > always reported as zero. This causes Xen to spurriously wake up
> > a domain even when the ring doesn't have enough space.
> >
> > This patch makes sure that the msg len is properly reported by
> > populating it in the event of a write failure.
>
> You're correct that this is an issue to be fixed, but unfortunately
> this patch doesn't compile, at least with gcc 8.2 with warnings as
> errors, reporting:
>
> argo.c: In function 'sendv':
> argo.c:2057:35: error: passing argument 3 of 'iov_count' from
> incompatible pointer type [-Werror=incompatible-pointer-types]
>              iov_count(iovs, niov, &len);
>                                    ^~~~
> argo.c:723:25: note: expected 'unsigned int *' but argument is of type
> 'long unsigned int *'
>            unsigned int *count)
>            ~~~~~~~~~~~~~~^~~~~

Shoot, sorry about that, it compiles on my end just fine.

> Even without this error, the logic it implements can unnecessarily
> invoke iov_count twice upon the same guest-supplied buffers; it would
> be better to avoid that, so: looking at the original section of code:
>
> * sendv's "len" variable can be int, rather than long.
> * iov_count can be called from sendv, just before ringbuf_insert,
> instead of within ringbuf_insert. It can populate sendv's "len"
> variable.
> * the len obtained from iov_count (if successful) can be passed into
> ringbuf_insert as a parameter, and replace ringbuf_insert's existing
> "len" variable.
> * ringbuf_insert's "out_len" pointer argument can then be dropped as
> unnecessary.
> * pending_requeue will be fine to use sendv's populated "len" variable.
>
> Christopher

This was an alternative that I had considered. Ultimately I went with my current
implementation as it had less of a SLOC change, though I see now that that was
a poor choice. Shall I submit as a v2 or reply to this thread
directly? Being that
the first patch was already pushed.

> > Signed-off-by: Nicholas Tsirakis <tsirakisn@ainfosec.com>
> > ---
> >  xen/common/argo.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/xen/common/argo.c b/xen/common/argo.c
> > index 2f874a570d..eb541829d6 100644
> > --- a/xen/common/argo.c
> > +++ b/xen/common/argo.c
> > @@ -2050,6 +2050,12 @@ sendv(struct domain *src_d, xen_argo_addr_t *src_addr,
> >          {
> >              int rc;
> >
> > +            /*
> > +             * if ringbuf_insert fails, then len will never be populated.
> > +             * make sure to populate it here.
> > +             */
> > +            iov_count(iovs, niov, &len);
> > +
> >              argo_dprintk("argo_ringbuf_sendv failed, EAGAIN\n");
> >              /* requeue to issue a notification when space is there */
> >              rc = pending_requeue(dst_d, ring_info, src_id.domain_id, len);
> > --
> > 2.17.1
> >

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] argo: correctly report pending message length

[Christopher Clark]

On Tue, Jun 11, 2019 at 12:55 PM Nicholas Tsirakis
<niko.tsirakis@gmail.com> wrote:
>
> On Tue, Jun 11, 2019 at 2:49 PM Christopher Clark
> <christopher.w.clark@gmail.com> wrote:
> >
> > On Tue, Jun 11, 2019 at 10:11 AM Nicholas Tsirakis
> > <niko.tsirakis@gmail.com> wrote:
> > >
> > > When a message is requeue'd in Xen's internal queue, the queue
> > > entry contains the length of the message so that Xen knows to
> > > send a VIRQ to the respective domain when enough space frees up
> > > in the ring. Due to a small bug, however, Xen doesn't populate
> > > the length of the msg if a given write fails, so this length is
> > > always reported as zero. This causes Xen to spurriously wake up
> > > a domain even when the ring doesn't have enough space.
> > >
> > > This patch makes sure that the msg len is properly reported by
> > > populating it in the event of a write failure.
> >
> > You're correct that this is an issue to be fixed, but unfortunately
> > this patch doesn't compile, at least with gcc 8.2 with warnings as
> > errors, reporting:
> >
> > argo.c: In function 'sendv':
> > argo.c:2057:35: error: passing argument 3 of 'iov_count' from
> > incompatible pointer type [-Werror=incompatible-pointer-types]
> >              iov_count(iovs, niov, &len);
> >                                    ^~~~
> > argo.c:723:25: note: expected 'unsigned int *' but argument is of type
> > 'long unsigned int *'
> >            unsigned int *count)
> >            ~~~~~~~~~~~~~~^~~~~
>
> Shoot, sorry about that, it compiles on my end just fine.
>
> > Even without this error, the logic it implements can unnecessarily
> > invoke iov_count twice upon the same guest-supplied buffers; it would
> > be better to avoid that, so: looking at the original section of code:
> >
> > * sendv's "len" variable can be int, rather than long.
> > * iov_count can be called from sendv, just before ringbuf_insert,
> > instead of within ringbuf_insert. It can populate sendv's "len"
> > variable.
> > * the len obtained from iov_count (if successful) can be passed into
> > ringbuf_insert as a parameter, and replace ringbuf_insert's existing
> > "len" variable.
> > * ringbuf_insert's "out_len" pointer argument can then be dropped as
> > unnecessary.
> > * pending_requeue will be fine to use sendv's populated "len" variable.
>
> This was an alternative that I had considered. Ultimately I went with my current
> implementation as it had less of a SLOC change, though I see now that that was
> a poor choice. Shall I submit as a v2 or reply to this thread
> directly? Being that
> the first patch was already pushed.

v2 please, ensuring that it applies to staging on top of the prior
patch already applied.

thanks

Christopher

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel