[RFC xserver] xwayland: HACK: Don't roundtrip in DRI2 authentication mechanism
Kristian Høgsberg
hoegsberg at gmail.com
Mon Jan 28 19:15:50 PST 2013
On Tue, Jan 22, 2013 at 08:19:12PM -0200, Tiago Vignatti wrote:
> Signed-off-by: Tiago Vignatti <tiago.vignatti at intel.com>
> ---
> hw/xfree86/dri2/dri2.c | 60 ++++++++++++++++++++++++++++++++++--
> hw/xfree86/dri2/dri2.h | 8 ++++-
> hw/xfree86/dri2/dri2ext.c | 10 +++++-
> hw/xfree86/xwayland/xwayland-drm.c | 22 +++++++++----
> 4 files changed, 89 insertions(+), 11 deletions(-)
>
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index b08618a..a529d5e 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -99,6 +99,7 @@ typedef struct _DRI2Screen {
> const char *deviceName;
> int fd;
> unsigned int lastSequence;
> + ClientPtr blockedAuthClient;
Nothing prevents multiple client from trying to authenticate at the
same time. We can't just support one blocked auth client at a time.
If another client initiates and auth sequence in the middle of another
clients auth sequence, you just overwrite blockedAuthClient in
DRI2Authenticate. This leaves the first client stuck in ignored mode.
> DRI2CreateBufferProcPtr CreateBuffer;
> DRI2DestroyBufferProcPtr DestroyBuffer;
> @@ -1122,13 +1123,65 @@ DRI2AuthMagic (ScreenPtr pScreen, uint32_t magic)
> return (*ds->LegacyAuthMagic) (ds->fd, magic);
> }
>
> +/*
> + * Return TRUE if it has a blocked client waiting authentication or
> + * FALSE otherwise.
> + */
> Bool
> -DRI2Authenticate(ScreenPtr pScreen, uint32_t magic)
> +DRI2HasBlockedAuthClient(ScreenPtr pScreen)
> {
> DRI2ScreenPtr ds = DRI2GetScreen(pScreen);
>
> - if (ds == NULL || (*ds->AuthMagic) (pScreen, magic))
> - return FALSE;
> + if (ds == NULL)
> + return FALSE;
> +
> + if (!ds->blockedAuthClient)
> + return FALSE;
> + else
> + return TRUE;
> +}
> +
> +/*
> + * Attend blocked client that was asking for authentication. In other words,
> + * it will process the DRI2Authenticate request again.
> + *
> + * This has to be called only if the client has been in fact authenticated by
> + * DRM, meaning the token magic was accepted.
> + */
> +void
> +DRI2AttendBlockedAuthClient(ScreenPtr pScreen)
> +{
> + DRI2ScreenPtr ds = DRI2GetScreen(pScreen);
> +
> + if (ds == NULL || !DRI2HasBlockedAuthClient(pScreen))
> + return;
> +
> + AttendClient(ds->blockedAuthClient);
> +}
> +
> +Bool
> +DRI2Authenticate(ScreenPtr pScreen, uint32_t magic, ClientPtr client)
> +{
> + DRI2ScreenPtr ds = DRI2GetScreen(pScreen);
> +
> + fprintf(stderr, "%s (blocked client: %p)\n", __func__, ds->blockedAuthClient);
> + if (ds == NULL)
> + return FALSE;
> +
> + if (DRI2HasBlockedAuthClient(pScreen) &&
> + ds->blockedAuthClient == client) {
> + ds->blockedAuthClient = NULL;
> + return TRUE;
> + }
> +
> + /* XXX: this assumes magic will never fail */
But it *can* fail. If the X server or wayland server is vt-switched
away, it can't authenticate new drm clients. Instead, we can use a
specific return code to indicate "please block the client". Currently
the return code convention for ds->AuthMagic is a little inconsistent.
In the non-xwayland case, the DDX returns the return value of
drmAuthMagic, which is 0 for success and -errno in case of error. In
the xwayland case, we return 0 for success or an X error code
(BadAlloc etc). We could use -EAGAIN as a AuthMagic return value to
indicate that the authentication is in progress.
But I think we may be better off just adding a new
DRI2AuthMagic3ProcPtr, that lets dri2.c pass the client to the auth
function. That way the auth function can just call IgnoreClient, and
dri2 can check client->ignoreCount > 0 to see whether or not it should
send a reply. The other point here is that when xwayland gets the
authenticate reply, it needs to call back into dri2.c to send the
reply and unblock the client and for that it needs to get the client
ptr in the first place.
Finally, we can't change DRI2Authenticate, since it's xserver ABI.
That doesn't mean we can't add a new function for ProcDRI2Authenticate
to call. This function will essentially look like DRI2Authenticate,
except it takes a ClientPtr as well and passes that to ds->AuthMagic3.
> + if ((*ds->AuthMagic) (pScreen, magic)) {
> + ResetCurrentRequest(client);
We don't need ResetCurrentRequest() after all. There's no need to
reset the request, we've processed it and we're just waiting to send
the reply. In other words all we need is
if (ds == NULL || (*ds->AuthMagic3) (pScreen, pClient, magic))
return FALSE;
> + client->sequence--;
> + IgnoreClient(client);
> + ds->blockedAuthClient = client;
> + return FALSE;
> + }
>
> return TRUE;
> }
> @@ -1198,6 +1251,7 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info)
> ds->screen = pScreen;
> ds->fd = info->fd;
> ds->deviceName = info->deviceName;
> + ds->blockedAuthClient = NULL;
> dri2_major = 1;
>
> ds->CreateBuffer = info->CreateBuffer;
> diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h
> index cec9634..195a253 100644
> --- a/hw/xfree86/dri2/dri2.h
> +++ b/hw/xfree86/dri2/dri2.h
> @@ -238,7 +238,13 @@ extern _X_EXPORT Bool DRI2Connect(ScreenPtr pScreen,
> const char **driverName,
> const char **deviceName);
>
> -extern _X_EXPORT Bool DRI2Authenticate(ScreenPtr pScreen, uint32_t magic);
> +extern _X_EXPORT Bool DRI2HasBlockedAuthClient(ScreenPtr pScreen);
> +
> +extern _X_EXPORT void DRI2AttendBlockedAuthClient(ScreenPtr pScreen);
> +
> +extern _X_EXPORT Bool DRI2Authenticate(ScreenPtr pScreen,
> + uint32_t magic,
> + ClientPtr client);
> extern _X_EXPORT int DRI2CreateDrawable(ClientPtr client,
> DrawablePtr pDraw,
> diff --git a/hw/xfree86/dri2/dri2ext.c b/hw/xfree86/dri2/dri2ext.c
> index 2579a5c..9f1cf0f 100644
> --- a/hw/xfree86/dri2/dri2ext.c
> +++ b/hw/xfree86/dri2/dri2ext.c
> @@ -149,8 +149,16 @@ ProcDRI2Authenticate(ClientPtr client)
> rep.type = X_Reply;
> rep.sequenceNumber = client->sequence;
> rep.length = 0;
> - rep.authenticated = DRI2Authenticate(pDraw->pScreen, stuff->magic);
> +
> + status = DRI2Authenticate(pDraw->pScreen, stuff->magic, client);
> + if (status != TRUE && DRI2HasBlockedAuthClient(pDraw->pScreen)) {
> + fprintf(stderr, "%s: client BLOCKED\n", __func__);
> + return Success;
> + }
Here we just check for client->ignoreCount > 0 and return Success if so.
> +
> + rep.authenticated = status;
> WriteToClient(client, sizeof(xDRI2AuthenticateReply), &rep);
> + fprintf(stderr, "%s: wrote back to client\n", __func__);
>
> return Success;
> }
> diff --git a/hw/xfree86/xwayland/xwayland-drm.c b/hw/xfree86/xwayland/xwayland-drm.c
> index c19eb84..4ca143d 100644
> --- a/hw/xfree86/xwayland/xwayland-drm.c
> +++ b/hw/xfree86/xwayland/xwayland-drm.c
> @@ -45,6 +45,7 @@
>
> #include "xwayland.h"
> #include "xwayland-private.h"
> +#include "../dri2/dri2.h"
>
> static void
> drm_handle_device (void *data, struct wl_drm *drm, const char *device)
> @@ -64,7 +65,13 @@ drm_handle_authenticated (void *data, struct wl_drm *drm)
> {
> struct xwl_screen *xwl_screen = data;
>
> + fprintf(stderr, "%s\n", __func__);
> xwl_screen->authenticated = 1;
> +
> + if (!xwl_screen->screen)
> + return;
> +
> + DRI2AttendBlockedAuthClient(xwl_screen->screen);
> }
> static const struct wl_drm_listener xwl_drm_listener =
> @@ -146,18 +153,21 @@ int xwl_screen_get_drm_fd(struct xwl_screen *xwl_screen)
> int xwl_drm_authenticate(struct xwl_screen *xwl_screen,
> uint32_t magic)
> {
> - int ret;
> -
> xwl_screen->authenticated = 0;
>
> if (xwl_screen->drm)
> wl_drm_authenticate (xwl_screen->drm, magic);
The wl_drm protocol is a little awkward here in that it doesn't tell
us which magic was authenticated in case we had multiple authenticate
requests in flight between X and the wayland server. But they're
always handled in sequence, so what we can do is to call
wl_drm_authenticate() followed by
callback = wl_display_sync(display);
wl_callback_add_listener(callback, &sync_listener, client);
and then in the sync_listener callback (see wl_display_roundtrip()
implementation for details) we can call DRI2SendAuthReply(client,
xwl_screen->authenticated). The sync callback also needs to set
xwl_screen->authenticated = 0;
now, since we only get the authenticated event if we successfully
authenticated. In case of failed authentication we don't get the
event. As such it's important to clear the flag so the next
authenticate transaction doesn't get a false positive.
The DRI2SendAuthReply() helper should send back the reply and call
AttendClient().
Kristian
> - ret = wl_display_roundtrip(xwl_screen->display);
> - if (ret == -1)
> - return BadAlloc;
> + /* at this point we would need to force a roundtrip to guarantee the
> + * client is authenticated by the compositor (xwl_screen->authenticated ==
> + * 1). But, assuming the WM is in the same process of Wayland compositor,
> + * this roundtrip cannot be performed because it might result in a
> + * deadlock state, for the case the WM is waiting for a reply on X server.
> + * Thus, the X server needs to return to the main loop, serve the WM,
> + * block X client and try the same request (DRI2Authenticate) later. */
> +
> if (!xwl_screen->authenticated)
> - return BadAlloc;
> + return BadAccess;
>
> return Success;
> }
> --
> 1.7.9.5
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list