| Message ID | 43766a806049b9556dd73ed8c1d6368ab2b26c4f.1560272437.git.tsirakisn@ainfosec.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] argo: warn sendv() caller when ring is full | expand |
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 >
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 > >
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
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);
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(+)