Message-ID: <CAH5fLgiVqSYcnS3b2=deGHg+VZk0RQK4HVBbrSrhxNMWYGUQ7w@mail.gmail.com>
Date: Thu, 11 Jul 2024 23:00:20 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Jocelyn Falempe <jfalempe@...hat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, 
	Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Wedson Almeida Filho <wedsonaf@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Bjorn Roy Baron <bjorn3_gh@...tonmail.com>, Benno Lossin <benno.lossin@...ton.me>, 
	Andreas Hindborg <a.hindborg@...sung.com>, linux-kernel@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, rust-for-linux@...r.kernel.org, 
	Danilo Krummrich <dakr@...hat.com>
Subject: Re: [PATCH v3 4/4] drm/panic: Add a QR code panic screen

On Wed, Jul 10, 2024 at 4:01 PM Jocelyn Falempe <jfalempe@...hat.com> wrote:
>
> This patch adds a new panic screen, with a QR code and the kmsg data
> embedded.
> If DRM_PANIC_SCREEN_QR_CODE_URL is set, then the kmsg data will be
> compressed with zlib and encoded as a numerical segment, and appended
> to the URL as a URL parameter. This allows to save space, and put
> about ~7500 bytes of kmsg data, in a V40 QR code.
> Linux distributions can customize the URL, and put a web frontend to
> directly open a bug report with the kmsg data.
>
> Otherwise the kmsg data will be encoded as a binary segment (ie raw
> ascii) and only a maximum of 2953 bytes of kmsg data will be
> available in the QR code.
>
> You can also limit the QR code size with DRM_PANIC_SCREEN_QR_VERSION.
>
> v2:
>  * Rewrite the rust comments with Markdown (Alice Ryhl)
>  * Mark drm_panic_qr_generate() as unsafe (Alice Ryhl)
>  * Use CStr directly, and remove the call to as_str_unchecked()
>    (Alice Ryhl)
>  * Add a check for data_len <= data_size (Greg KH)
>
> v3:
>  * Fix all rust comments (typo, punctuation) (Miguel Ojeda)
>  * Change the wording of safety comments (Alice Ryhl)
>  * Add a link to the javascript decoder in the Kconfig (Greg KH)
>  * Fix data_size and tmp_size check in drm_panic_qr_generate()
>
> Signed-off-by: Jocelyn Falempe <jfalempe@...hat.com>
> ---

Overall, it looks reasonable to me. Some comments below.

The changelog should go below the --- or in the cover letter.

> +       if (stream.total_out > max_qr_data_size) {
> +               /* too much data for the QR code, so skip the first line and try again */
> +               kmsg = strchr(kmsg + 1, '\n');
> +               if (!kmsg)
> +                       return -EINVAL;
> +               kmsg_len = strlen(kmsg);
> +               goto try_again;

It seems like kmsg will start with a newline character when this retry
routine runs. Is that really what you want? Why not instead strchr and
then do the plus one afterwards?

This would also simplify the logic for why `kmsg + 1` doesn't go out
of bounds. Right now I have to check that there's no codepath where
kmsg points at the nul terminator byte.

> +const __LOG_PREFIX: &[u8] = b"rust_qrcode\0";

I guess this constant is because you're not using the module! macro?

> +/// C entry point for the rust QR Code generator.
> +///
> +/// Write the QR code image in the data buffer, and return the QR code width,
> +/// or 0, if the data doesn't fit in a QR code.
> +///
> +/// * `url`: The base URL of the QR code. It will be encoded as Binary segment.
> +/// * `data`: A pointer to the binary data, to be encoded. if URL is NULL, it
> +///    will be encoded as binary segment, otherwise it will be encoded
> +///    efficiently as a numeric segment, and appended to the URL.
> +/// * `data_len`: Length of the data, that needs to be encoded, must be less
> +///    than data_size.
> +/// * `data_size`: Size of data buffer, it should be at least 4071 bytes to hold
> +///    a V40 QR code. It will then be overwritten with the QR code image.
> +/// * `tmp`: A temporary buffer that the QR code encoder will use, to write the
> +///    segments and ECC.
> +/// * `tmp_size`: Size of the temporary buffer, it must be at least 3706 bytes
> +///    long for V40.
> +///
> +/// # Safety
> +///
> +/// * `url` must be null or point at a nul-terminated string.
> +/// * `data` must be valid for reading and writing for `data_size` bytes.
> +/// * `tmp` must be valid for reading and writing for `tmp_size` bytes.
> +///
> +/// They must remain valid for the duration of the function call.
> +
> +#[no_mangle]
> +pub unsafe extern "C" fn drm_panic_qr_generate(
> +    url: *const i8,
> +    data: *mut u8,
> +    data_len: usize,
> +    data_size: usize,
> +    tmp: *mut u8,
> +    tmp_size: usize,
> +) -> u8 {
> +    if data_size < 4071 || tmp_size < 3706 || data_len > data_size {
> +        return 0;
> +    }
> +    // SAFETY: The caller ensures that `data` is a valid pointer for reading and
> +    // writing `data_size` bytes.
> +    let data_slice: &mut [u8] = unsafe { core::slice::from_raw_parts_mut(data, data_size) };
> +    // SAFETY: The caller ensures that `tmp` is a valid pointer for reading and
> +    // writing `tmp_size` bytes.
> +    let tmp_slice: &mut [u8] = unsafe { core::slice::from_raw_parts_mut(tmp, tmp_size) };
> +    if url.is_null() {
> +        match EncodedMsg::new(&[&Segment::Binary(&data_slice[0..data_len])], tmp_slice) {
> +            None => 0,
> +            Some(em) => {
> +                let qr_image = QrImage::new(&em, data_slice);
> +                qr_image.width
> +            }
> +        }
> +    } else {
> +        // SAFETY: The caller ensures that `url` is a valid pointer to a
> +        // nul-terminated string.
> +        let url_cstr: &CStr = unsafe { CStr::from_char_ptr(url) };
> +        let segments = &[
> +            &Segment::Binary(url_cstr.as_bytes()),
> +            &Segment::Numeric(&data_slice[0..data_len]),
> +        ];
> +        match EncodedMsg::new(segments, tmp_slice) {
> +            None => 0,
> +            Some(em) => {
> +                let qr_image = QrImage::new(&em, data_slice);
> +                qr_image.width
> +            }
> +        }
> +    }
> +}

This looks good to me. :)

Alice

Powered by blists - more mailing lists