| Message ID | 20161024231934.2243-4-robert@sixbynine.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 25 October 2016 at 00:19, Robert Bragg <robert@sixbynine.org> wrote: > check_cmd() is checking whether a command adheres to certain > restrictions that ensure it's safe to execute within a privileged batch > buffer. Returning false implies a privilege problem, not that the > command is invalid. > > The distinction makes the difference between allowing the buffer to be > executed as an unprivileged batch buffer or returning an EINVAL error to > userspace without executing anything. > > In a case where userspace may want to test whether it can successfully > write to a register that needs privileges the distinction may be > important and an EINVAL error may be considered fatal. > > In particular this is currently true for Mesa, which includes a test for > whether OACONTROL can be written too, but Mesa treats any error when > flushing a batch buffer as fatal, calling exit(1). > > As it is currently Mesa can gracefully handle a failure to write to > OACONTROL if the command parser is disabled, but if we were to remove > OACONTROL from the parser's whitelist then the returned EINVAL would > break Mesa applications as they attempt an OACONTROL write. > > This bumps the command parser version from 7 to 8, as the change is > visible to userspace. > > Signed-off-by: Robert Bragg <robert@sixbynine.org> Seems reasonable. Reviewed-by: Matthew Auld <matthew.auld@intel.com>
diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c index fe34470..c45dd83 100644 --- a/drivers/gpu/drm/i915/i915_cmd_parser.c +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c @@ -1272,7 +1272,7 @@ int intel_engine_cmd_parser(struct intel_engine_cs *engine, if (!check_cmd(engine, desc, cmd, length, is_master, &oacontrol_set)) { - ret = -EINVAL; + ret = -EACCES; break; } @@ -1333,6 +1333,9 @@ int i915_cmd_parser_get_version(struct drm_i915_private *dev_priv) * 5. GPGPU dispatch compute indirect registers. * 6. TIMESTAMP register and Haswell CS GPR registers * 7. Allow MI_LOAD_REGISTER_REG between whitelisted registers. + * 8. Don't report cmd_check() failures as EINVAL errors to userspace; + * rely on the HW to NOOP disallowed commands as it would without + * the parser enabled. */ - return 7; + return 8; }
check_cmd() is checking whether a command adheres to certain restrictions that ensure it's safe to execute within a privileged batch buffer. Returning false implies a privilege problem, not that the command is invalid. The distinction makes the difference between allowing the buffer to be executed as an unprivileged batch buffer or returning an EINVAL error to userspace without executing anything. In a case where userspace may want to test whether it can successfully write to a register that needs privileges the distinction may be important and an EINVAL error may be considered fatal. In particular this is currently true for Mesa, which includes a test for whether OACONTROL can be written too, but Mesa treats any error when flushing a batch buffer as fatal, calling exit(1). As it is currently Mesa can gracefully handle a failure to write to OACONTROL if the command parser is disabled, but if we were to remove OACONTROL from the parser's whitelist then the returned EINVAL would break Mesa applications as they attempt an OACONTROL write. This bumps the command parser version from 7 to 8, as the change is visible to userspace. Signed-off-by: Robert Bragg <robert@sixbynine.org> --- drivers/gpu/drm/i915/i915_cmd_parser.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)