When doing an atomic modeset with ALLOW_MODESET drivers are allowed to
pull in arbitrary other resources, including CRTCs (e.g. when
reconfiguring global resources).
But in nonblocking mode userspace has then no idea this happened,
which can lead to spurious EBUSY calls, both:
- when that other CRTC is currently busy doing a page_flip the
ALLOW_MODESET commit can fail with an EBUSY
- on the other CRTC a normal atomic flip can fail with EBUSY because
of the additional commit inserted by the kernel without userspace's
knowledge
For blocking commits this isn't a problem, because everyone else will
just block until all the CRTC are reconfigured. Only thing userspace
can notice is the dropped frames without any reason for why frames got
dropped.
Consensus is that we need new uapi to handle this properly, but no one
has any idea what exactly the new uapi should look like. As a stop-gap
plug this problem by demoting nonblocking commits which might cause
issues by including CRTCs not in the original request to blocking
commits.
v2: Add comments and a WARN_ON to enforce this only when allowed - we
don't want to silently convert page flips into blocking plane updates
just because the driver is buggy.
v3: Fix inverted WARN_ON (Pekka).
References: https://lists.freedesktop.org/archives/dri-devel/2018-July/182281.html
Bugzilla: https://gitlab.freedesktop.org/wayland/weston/issues/24#note_9568
Cc: Daniel Stone <daniel(a)fooishbar.org>
Cc: Pekka Paalanen <pekka.paalanen(a)collabora.co.uk>
Cc: stable(a)vger.kernel.org
Reviewed-by: Daniel Stone <daniels(a)collabora.com>
Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com>
---
drivers/gpu/drm/drm_atomic.c | 34 +++++++++++++++++++++++++++++++---
1 file changed, 31 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 9ccfbf213d72..4c035abf98b8 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1362,15 +1362,43 @@ EXPORT_SYMBOL(drm_atomic_commit);
int drm_atomic_nonblocking_commit(struct drm_atomic_state *state)
{
struct drm_mode_config *config = &state->dev->mode_config;
- int ret;
+ unsigned requested_crtc = 0;
+ unsigned affected_crtc = 0;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *crtc_state;
+ bool nonblocking = true;
+ int ret, i;
+
+ /*
+ * For commits that allow modesets drivers can add other CRTCs to the
+ * atomic commit, e.g. when they need to reallocate global resources.
+ *
+ * But when userspace also requests a nonblocking commit then userspace
+ * cannot know that the commit affects other CRTCs, which can result in
+ * spurious EBUSY failures. Until we have better uapi plug this by
+ * demoting such commits to blocking mode.
+ */
+ for_each_new_crtc_in_state(state, crtc, crtc_state, i)
+ requested_crtc |= drm_crtc_mask(crtc);
ret = drm_atomic_check_only(state);
if (ret)
return ret;
- DRM_DEBUG_ATOMIC("committing %p nonblocking\n", state);
+ for_each_new_crtc_in_state(state, crtc, crtc_state, i)
+ affected_crtc |= drm_crtc_mask(crtc);
+
+ if (affected_crtc != requested_crtc) {
+ /* adding other CRTC is only allowed for modeset commits */
+ WARN_ON(!state->allow_modeset);
+
+ DRM_DEBUG_ATOMIC("demoting %p to blocking mode to avoid EBUSY\n", state);
+ nonblocking = false;
+ } else {
+ DRM_DEBUG_ATOMIC("committing %p nonblocking\n", state);
+ }
- return config->funcs->atomic_commit(state->dev, state, true);
+ return config->funcs->atomic_commit(state->dev, state, nonblocking);
}
EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
--
2.24.1
Since commit 8df9ffb888c ("can: make use of preallocated can_ml_priv for per
device struct can_dev_rcv_lists") the device specific CAN receive filter lists
are stored in netdev_priv() and dev->ml_priv points to these filters.
In the bug report Syzkaller enslaved a vxcan1 CAN device and accessed the
bonding device with a PF_CAN socket which lead to a crash due to an access of
an unhandled bond_dev->ml_priv pointer.
Deny to enslave CAN devices by the bonding driver as the resulting bond_dev
pretends to be a CAN device by copying dev->type without really being one.
Reported-by: syzbot+c3ea30e1e2485573f953(a)syzkaller.appspotmail.com
Fixes: 8df9ffb888c ("can: make use of preallocated can_ml_priv for per
device struct can_dev_rcv_lists")
Cc: linux-stable <stable(a)vger.kernel.org> # >= v5.4
Signed-off-by: Oliver Hartkopp <socketcan(a)hartkopp.net>
---
drivers/net/bonding/bond_main.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 48d5ec770b94..4b781a7dfd96 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1475,6 +1475,18 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
return -EPERM;
}
+ /* CAN network devices hold device specific filter lists in
+ * netdev_priv() where dev->ml_priv sets a reference to.
+ * As bonding assumes to have some ethernet-like device it doesn't
+ * take care about these CAN specific filter lists today.
+ * So we deny the enslaving of CAN interfaces here.
+ */
+ if (slave_dev->type == ARPHRD_CAN) {
+ NL_SET_ERR_MSG(extack, "CAN devices can not be enslaved");
+ slave_err(bond_dev, slave_dev, "no bonding on CAN devices\n");
+ return -EINVAL;
+ }
+
/* set bonding device ether type by slave - bonding netdevices are
* created with ether_setup, so when the slave type is not ARPHRD_ETHER
* there is a need to override some of the type dependent attribs/funcs.
--
2.20.1