xen/arch/x86/irq.c | 6 +- xen/arch/x86/pv/shim.c | 9 ++- xen/common/event_channel.c | 144 +++++++++++++++++++----------------- xen/common/event_fifo.c | 25 +++++-- xen/include/xen/event.h | 29 ++++++-- xen/include/xen/sched.h | 8 +- xen/include/xsm/xsm.h | 1 - xen/xsm/flask/avc.c | 78 ++----------------- xen/xsm/flask/hooks.c | 10 --- xen/xsm/flask/include/avc.h | 2 - 10 files changed, 139 insertions(+), 173 deletions(-)
The patches for XSA-343 produced some fallout, especially the event channel locking has shown to be problematic. Patch 1 is targeting fifo event channels for avoiding any races for the case that the fifo queue has been changed for a specific event channel. The second patch is modifying the per event channel locking scheme in order to avoid deadlocks and problems due to the event channel lock having been changed to require IRQs off by the XSA-343 patches. Changes in V6: - added patch 3 (Jan Beulich) - switched some more read_trylock() cases to read_lock() (Jan Beulich) Changes in V5: - moved evtchn_write_[un]lock() to event_channel.c (Jan Beulich) - used normal read_lock() in some cases (Jan Beulich) Changes in V4: - switched to real rwlock Changes in V3: - addressed comments Juergen Gross (3): xen/events: access last_priority and last_vcpu_id together xen/evtchn: rework per event channel lock xen/evtchn: revert 52e1fc47abc3a0123 xen/arch/x86/irq.c | 6 +- xen/arch/x86/pv/shim.c | 9 ++- xen/common/event_channel.c | 144 +++++++++++++++++++----------------- xen/common/event_fifo.c | 25 +++++-- xen/include/xen/event.h | 29 ++++++-- xen/include/xen/sched.h | 8 +- xen/include/xsm/xsm.h | 1 - xen/xsm/flask/avc.c | 78 ++----------------- xen/xsm/flask/hooks.c | 10 --- xen/xsm/flask/include/avc.h | 2 - 10 files changed, 139 insertions(+), 173 deletions(-) -- 2.26.2
Hi, On 09/11/2020 16:38, Juergen Gross wrote: > The patches for XSA-343 produced some fallout, especially the event > channel locking has shown to be problematic. > > Patch 1 is targeting fifo event channels for avoiding any races for the > case that the fifo queue has been changed for a specific event channel. > > The second patch is modifying the per event channel locking scheme in > order to avoid deadlocks and problems due to the event channel lock > having been changed to require IRQs off by the XSA-343 patches. > > Changes in V6: > - added patch 3 (Jan Beulich) > - switched some more read_trylock() cases to read_lock() (Jan Beulich) > > Changes in V5: > - moved evtchn_write_[un]lock() to event_channel.c (Jan Beulich) > - used normal read_lock() in some cases (Jan Beulich) > > Changes in V4: > - switched to real rwlock > > Changes in V3: > - addressed comments > > Juergen Gross (3): > xen/events: access last_priority and last_vcpu_id together > xen/evtchn: rework per event channel lock > xen/evtchn: revert 52e1fc47abc3a0123 While looking at the list of commits, I noticed that the first patch hasn't been committed. They were all acked/reviewed, so I am a bit puzzled why this was omitted... I have nearly missed as I was expecting the 3 patches to be committed together. May I suggest that in the future we reply to the cover letter and mention which patches are (or not) committed? Regarding patch #1, I will commit it tomorrow unless there are strong objections against. Cheers, -- Julien Grall
On 17.11.2020 19:13, Julien Grall wrote: > On 09/11/2020 16:38, Juergen Gross wrote: >> Juergen Gross (3): >> xen/events: access last_priority and last_vcpu_id together >> xen/evtchn: rework per event channel lock >> xen/evtchn: revert 52e1fc47abc3a0123 > > While looking at the list of commits, I noticed that the first patch > hasn't been committed. They were all acked/reviewed, so I am a bit > puzzled why this was omitted... > > I have nearly missed as I was expecting the 3 patches to be committed > together. May I suggest that in the future we reply to the cover letter > and mention which patches are (or not) committed? > > Regarding patch #1, I will commit it tomorrow unless there are strong > objections against. Without a clear outline of what would break with the present logic, I had previously indicated I'm not convinced of the change. This isn't a strong objection, no, but I still wouldn't want to see my name associated with it in such a case. Furthermore I clearly view this as not a backporting candidate, while the other two are (as I did previously indicate). Hence the latter two patches wanted re-basing ahead of the first one anyway, to ease the backports. While I will accept there are different views possible here, I also don't think sending mail in such a case is a good use of resources. The information what was or was not committed is readily available. Personally I view such mails as at least very close to spam. Irrespective of the above I'm sorry for any inconvenience caused. Jan
Hi Jan, On 18/11/2020 08:22, Jan Beulich wrote: > On 17.11.2020 19:13, Julien Grall wrote: >> On 09/11/2020 16:38, Juergen Gross wrote: >>> Juergen Gross (3): >>> xen/events: access last_priority and last_vcpu_id together >>> xen/evtchn: rework per event channel lock >>> xen/evtchn: revert 52e1fc47abc3a0123 >> >> While looking at the list of commits, I noticed that the first patch >> hasn't been committed. They were all acked/reviewed, so I am a bit >> puzzled why this was omitted... >> >> I have nearly missed as I was expecting the 3 patches to be committed >> together. May I suggest that in the future we reply to the cover letter >> and mention which patches are (or not) committed? >> >> Regarding patch #1, I will commit it tomorrow unless there are strong >> objections against. > > Without a clear outline of what would break with the present logic, > I had previously indicated I'm not convinced of the change. This > isn't a strong objection, no, but I still wouldn't want to see my > name associated with it in such a case. I was under the impression that the committer job is mostly mechanical (i.e. collecting tags and applying patches). There are no rules in MAINTAINERS that mention committers can decide what gets committed as long as maintainers approved it and there are no strong objections. > Furthermore I clearly view > this as not a backporting candidate, while the other two are (as I > did previously indicate). Hence the latter two patches wanted > re-basing ahead of the first one anyway, to ease the backports. I understand the backport concern. However, if there were clash, then it means you had to resolve them on commit to staging. Surely, they were quite minimal otherwise you would have sent an e-mail on xen-devel, right? > While I will accept there are different views possible here, I also > don't think sending mail in such a case is a good use of resources. > The information what was or was not committed is readily available. > Personally I view such mails as at least very close to spam. This is a matter of perspective. It helps to confirm with the contributor that it was fine to merge only part of the series (multiple pair of eyes is always better than one...) or mentioning any clash/reworked. It also makes easier for reviewers to figure out what was committed as it can be difficult to know if a patch was merged because commit title can be altered (even simply dropping the prefix "xen/" can take a coule of more minutes to pinpoint commit). Therefore, I think there is a value for such e-mail to be sent out. It will likely improve coordination among the member of the community. I would be happy to consider different approach if it fullfills that goal. Cheers, -- Julien Grall
Julien Grall writes ("Re: [PATCH v6 0/3] XSA-343 followup patches"):
> On 18/11/2020 08:22, Jan Beulich wrote:
> > Without a clear outline of what would break with the present logic,
> > I had previously indicated I'm not convinced of the change. This
> > isn't a strong objection, no, but I still wouldn't want to see my
> > name associated with it in such a case.
>
> I was under the impression that the committer job is mostly mechanical
> (i.e. collecting tags and applying patches). There are no rules in
> MAINTAINERS that mention committers can decide what gets committed as
> long as maintainers approved it and there are no strong objections.
I think in practice committers need to exercise quite a bit of
judgement. I often find myself deciding on what seem to be edge cases
of the rules. I also sometimes, with something which has the formally
needed acks, but which seems like it might be controversial or
awkward, decide to just make an extra double-check. I sometimes even
commit something when maybe the formal rules wouldn't permit it, when
I'm confident that the relevant maintainer(s) etc. wouldn't object -
an example being when something is badly broken and this is allegedly
the fix.
However, I don't agree that a committer should omit committing
something which is acked, and is part of a series which they are
committing, simply because they don't see the need for the patch.
If a committer who is also a maintainer has a firm objection, they
should state that objection as a formal NAK. If a formal NAK is not
warranted, a committer should not engage in passive obstruction.
Also, a committer should not "silently" commit only part of a series.
In summary, committing something is not a declaration by the committer
that they approve of the patch on a technical level.
It is a declaration that (in the committer's view) the patch is
properly acked/approved, and that therefore according to our shared
understanding of the community's processes, it ought to go in.
That might even occur if the committer has an outstanding technical
objection. I have certainly committed things that I didn't really
like very much, on the grounds that they had enough acks and that I
didn't feel I wanted to give it a formal NAK.
If it would help a committer feel more comfortable to be explicit
about this, there are a number of approaches available: the committer
could commit the patch but also reply to it on-list restating their
objection but saying that they are committing it despite the
objection, because of others' acks.
If it is felt desirable to record this information in the repository,
one could write something like
Not-Acked-by: Ian Jackson <iwj@xenproject.org>
(which I think is not the same as Nacked-by) or even add a note in
the commit message like
[ I am committing this, even though I don't think it is
necessary, because it has the requisite acks. I do not
think it warrants a nack. -iwj ]
> This is a matter of perspective. It helps to confirm with the
> contributor that it was fine to merge only part of the series
> (multiple pair of eyes is always better than one...) or mentioning
> any clash/reworked.
I think it is especially important to send an email about things being
committed when what has happened might be surprising. For example, if
only part of a series is committed.
Thanks,
Ian.
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Julien Grall > Sent: 18 November 2020 14:18 > To: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Ian Jackson > <iwj@xenproject.org>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Roger Pau > Monné <roger.pau@citrix.com>; Daniel De Graaf <dgdegra@tycho.nsa.gov>; committers@xenproject.org; xen- > devel@lists.xenproject.org; Juergen Gross <jgross@suse.com> > Subject: Re: [PATCH v6 0/3] XSA-343 followup patches > > Hi Jan, > > On 18/11/2020 08:22, Jan Beulich wrote: > > On 17.11.2020 19:13, Julien Grall wrote: > >> On 09/11/2020 16:38, Juergen Gross wrote: > >>> Juergen Gross (3): > >>> xen/events: access last_priority and last_vcpu_id together > >>> xen/evtchn: rework per event channel lock > >>> xen/evtchn: revert 52e1fc47abc3a0123 > >> > >> While looking at the list of commits, I noticed that the first patch > >> hasn't been committed. They were all acked/reviewed, so I am a bit > >> puzzled why this was omitted... > >> > >> I have nearly missed as I was expecting the 3 patches to be committed > >> together. May I suggest that in the future we reply to the cover letter > >> and mention which patches are (or not) committed? > >> > >> Regarding patch #1, I will commit it tomorrow unless there are strong > >> objections against. > > > > Without a clear outline of what would break with the present logic, > > I had previously indicated I'm not convinced of the change. This > > isn't a strong objection, no, but I still wouldn't want to see my > > name associated with it in such a case. > > I was under the impression that the committer job is mostly mechanical > (i.e. collecting tags and applying patches). There are no rules in > MAINTAINERS that mention committers can decide what gets committed as > long as maintainers approved it and there are no strong objections. > > > Furthermore I clearly view > > this as not a backporting candidate, while the other two are (as I > > did previously indicate). Hence the latter two patches wanted > > re-basing ahead of the first one anyway, to ease the backports. > > I understand the backport concern. However, if there were clash, then it > means you had to resolve them on commit to staging. Surely, they were > quite minimal otherwise you would have sent an e-mail on xen-devel, right? > > > While I will accept there are different views possible here, I also > > don't think sending mail in such a case is a good use of resources. > > The information what was or was not committed is readily available. > Personally I view such mails > as at least very close to spam. > > This is a matter of perspective. It helps to confirm with the > contributor that it was fine to merge only part of the series (multiple > pair of eyes is always better than one...) or mentioning any clash/reworked. > > It also makes easier for reviewers to figure out what was committed as > it can be difficult to know if a patch was merged because commit title > can be altered (even simply dropping the prefix "xen/" can take a coule > of more minutes to pinpoint commit). > > Therefore, I think there is a value for such e-mail to be sent out. It > will likely improve coordination among the member of the community. +1 Knowing that part of a long series has already been committed would be useful. When I do the usual pull/rebase dance prior to re-submitting I have, more than once, been surprised by rebase failures because some of the patches had been committed but were modified in doing do. It's not a massive problem but an email would have at least made me aware I needed to be a bit more careful. Paul > > I would be happy to consider different approach if it fullfills that goal. > > Cheers, > > -- > Julien Grall
On 18.11.20 09:22, Jan Beulich wrote: > On 17.11.2020 19:13, Julien Grall wrote: >> On 09/11/2020 16:38, Juergen Gross wrote: >>> Juergen Gross (3): >>> xen/events: access last_priority and last_vcpu_id together >>> xen/evtchn: rework per event channel lock >>> xen/evtchn: revert 52e1fc47abc3a0123 >> >> While looking at the list of commits, I noticed that the first patch >> hasn't been committed. They were all acked/reviewed, so I am a bit >> puzzled why this was omitted... >> >> I have nearly missed as I was expecting the 3 patches to be committed >> together. May I suggest that in the future we reply to the cover letter >> and mention which patches are (or not) committed? >> >> Regarding patch #1, I will commit it tomorrow unless there are strong >> objections against. > > Without a clear outline of what would break with the present logic, > I had previously indicated I'm not convinced of the change. This > isn't a strong objection, no, but I still wouldn't want to see my > name associated with it in such a case. Furthermore I clearly view > this as not a backporting candidate, while the other two are (as I > did previously indicate). Hence the latter two patches wanted > re-basing ahead of the first one anyway, to ease the backports. Consider an NMI during evtchn_fifo_set_pending() between updating last_vcpu_id and last_priority, while on another cpu a concurrent evtchn_fifo_set_pending() is being called. On that other cpu lock_old_queue() might return a wrong queue as it will read only the new last_vcpu_id, but not the new last_priority value. Juergen
On 18.11.2020 09:41, Jürgen Groß wrote: > On 18.11.20 09:22, Jan Beulich wrote: >> On 17.11.2020 19:13, Julien Grall wrote: >>> On 09/11/2020 16:38, Juergen Gross wrote: >>>> Juergen Gross (3): >>>> xen/events: access last_priority and last_vcpu_id together >>>> xen/evtchn: rework per event channel lock >>>> xen/evtchn: revert 52e1fc47abc3a0123 >>> >>> While looking at the list of commits, I noticed that the first patch >>> hasn't been committed. They were all acked/reviewed, so I am a bit >>> puzzled why this was omitted... >>> >>> I have nearly missed as I was expecting the 3 patches to be committed >>> together. May I suggest that in the future we reply to the cover letter >>> and mention which patches are (or not) committed? >>> >>> Regarding patch #1, I will commit it tomorrow unless there are strong >>> objections against. >> >> Without a clear outline of what would break with the present logic, >> I had previously indicated I'm not convinced of the change. This >> isn't a strong objection, no, but I still wouldn't want to see my >> name associated with it in such a case. Furthermore I clearly view >> this as not a backporting candidate, while the other two are (as I >> did previously indicate). Hence the latter two patches wanted >> re-basing ahead of the first one anyway, to ease the backports. > > Consider an NMI during evtchn_fifo_set_pending() between updating > last_vcpu_id and last_priority, while on another cpu a concurrent > evtchn_fifo_set_pending() is being called. On that other cpu > lock_old_queue() might return a wrong queue as it will read only > the new last_vcpu_id, but not the new last_priority value. Which has become possible only as of patch 2 of this series, i.e. this one really was a prereq without its description making this clear. Now that it'll go in on top of the other two, there's no strict need to change the description anymore, though. But I take from this that I should also queue it up for backporting then. Jan
© 2016 - 2024 Red Hat, Inc.