Add IOCTLs to the dummy Gramine device

[dimakuv]

New IOCTLs (simple string manipulations):

• GRAMINE_IOCTL_REWIND
• GRAMINE_IOCTL_WRITE
• GRAMINE_IOCTL_READ
• GRAMINE_IOCTL_GETSIZE
• GRAMINE_IOCTL_CLEAR
• GRAMINE_IOCTL_REPLACE_ARR
• GRAMINE_IOCTL_REPLACE_LIST

Companion PR: gramineproject/gramine#671

---

This change is 

[dimakuv]

Reviewable status: 0 of 2 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

---

gramine-device-testing-module/ioctl.h line 36 at r1 (raw file):

    /* list of replacements, e.g. [`l` -> `$`, next points to `o` -> `0`, next points to NULL] */
    struct gramine_ioctl_replace_char __user* replacements_list;
};

FYI: This struct could be removed as it only wraps another struct. But I decided to keep like this because we may add more fields to this "wrapping" struct, so this level of indirection looks future-proof.

[boryspoplawski]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)

a discussion (no related file):
I would replace gramine_ioctl with gramine_test_dev_ioctl eveywhere

---

---

-- commits line 2 at r1:
This is not-so-dummy anymore

Suggestion:

Add `ioctl` to the Gramine test device

---

gramine-device-testing-module/ioctl.h line 1 at r1 (raw file):

#ifndef IOCTL_H

#pragma once

---

gramine-device-testing-module/ioctl.h line 1 at r1 (raw file):

#ifndef IOCTL_H

Please add some prefix to this file (e.g. gramine_test_dev_ioctl.h)

---

gramine-device-testing-module/ioctl.h line 6 at r1 (raw file):

#include <linux/ioctl.h>

struct gramine_ioctl_write {

What's the point of these read/write ioctls? I would expect we need to test read/write interaction with ioctl too, so we would always use them (over these two ioctls)

---

gramine-device-testing-module/ioctl.h line 9 at r1 (raw file):

    size_t buf_size;        /* in */
    const char __user* buf; /* in */
    loff_t off;             /* in/out -- updated after write */

What's the point of updating this? Isn't always copied just added to it?

---

gramine-device-testing-module/ioctl.h line 16 at r1 (raw file):

    size_t buf_size;        /* in */
    char __user* buf;       /* out */
    loff_t off;             /* in/out -- updated after read */

ditto

---

gramine-device-testing-module/ioctl.h line 24 at r1 (raw file):

    char dst;               /* in */
    char pad[6];
    struct gramine_ioctl_replace_char __user* next; /* unused for array, used for list */

Why this is here then? Why not have struct gramine_ioctl_replace_list __user* next in list struct? See the comment there.

---

gramine-device-testing-module/ioctl.h line 36 at r1 (raw file):

    /* list of replacements, e.g. [`l` -> `$`, next points to `o` -> `0`, next points to NULL] */
    struct gramine_ioctl_replace_char __user* replacements_list;
};

Why not like this?

Suggestion:

struct gramine_ioctl_replace_list {
    /* list of replacements, e.g. [`l` -> `$`, next points to `o` -> `0`, next points to NULL] */
    struct gramine_ioctl_replace_char replacement;
    struct gramine_ioctl_replace_list __user* next;
};

---

gramine-device-testing-module/ioctl.h line 38 at r1 (raw file):

};

#define GRAMINE_IOCTL_BASE 0x33

Out of curiosity, why 0x33? :)

---

gramine-device-testing-module/main.c line 36 at r1 (raw file):

static void replace_all_occurences(struct gramine_test_dev_data* data, char src, char dst) {
    size_t idx = 0;
    while (idx < data->size) {

Why not for?

---

gramine-device-testing-module/main.c line 37 at r1 (raw file):

    size_t idx = 0;
    while (idx < data->size) {
        if (data->buf[idx] == '\0')

Why this check?

---

gramine-device-testing-module/main.c line 130 at r1 (raw file):

    void __user* argp_user = (void __user*)argp;

    switch (cmd) {

Please create dedicated functions for the more complex ioctls

---

gramine-device-testing-module/main.c line 146 at r1 (raw file):

            if (copy_to_user(argp_user, &arg, sizeof(arg))) {
                return -EFAULT;
            }

This does not update file offset.

---

gramine-device-testing-module/main.c line 162 at r1 (raw file):

            if (copy_to_user(argp_user, &arg, sizeof(arg))) {
                return -EFAULT;
            }

ditto (file offset not updated)

---

gramine-device-testing-module/main.c line 180 at r1 (raw file):

            }
            replace_chars = arg.replacements_arr;
            for (i = 0; i < arg.replacements_cnt; i++) {

This cannot be done the to C version used, right?

Suggestion:

size_t i = 0;

---

gramine-device-testing-module/main.c line 182 at r1 (raw file):

            for (i = 0; i < arg.replacements_cnt; i++) {
                struct gramine_ioctl_replace_char replace_char;
                if (copy_from_user(&replace_char, replace_chars, sizeof(replace_char))) {

This replace_char vs replace_chars is a bit confusing.
I would suggest removing replace_chars and:

Suggestion:

copy_from_user(&replace_char, &arg.replacement_arr[i], sizeof(replace_char))

[dimakuv]

Reviewable status: 0 of 4 files reviewed, 16 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

I would replace gramine_ioctl with gramine_test_dev_ioctl eveywhere

Done

---

a discussion (no related file):
FYI: Also updated gramineproject/gramine#671 accordingly.

---

---

-- commits line 2 at r1:

Previously, boryspoplawski (Borys Popławski) wrote…

This is not-so-dummy anymore

Will do during the final rebase. Blocked myself.

---

-- commits line 11 at r1:
All these should be renamed to GRAMINE_TEST_DEV_IOCTL_...

---

gramine-device-testing-module/main.c line 36 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why not for?

Done.

---

gramine-device-testing-module/main.c line 37 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why this check?

Done. Wanted to make it very explicit that we replace only until the end of the string, but you're right, we can just rely on data->size.

---

gramine-device-testing-module/main.c line 130 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Please create dedicated functions for the more complex ioctls

I'm postponing this until you are happy with the other comments -- otherwise it will be hard to review.

---

gramine-device-testing-module/main.c line 146 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This does not update file offset.

Done.

---

gramine-device-testing-module/main.c line 162 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

ditto (file offset not updated)

Done.

---

gramine-device-testing-module/main.c line 180 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This cannot be done the to C version used, right?

Yes, exactly.

---

gramine-device-testing-module/main.c line 182 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This replace_char vs replace_chars is a bit confusing.
I would suggest removing replace_chars and:

Done.

---

gramine-device-testing-module/ioctl.h line 1 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Please add some prefix to this file (e.g. gramine_test_dev_ioctl.h)

Done.

---

gramine-device-testing-module/ioctl.h line 1 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

#pragma once

Done.

---

gramine-device-testing-module/ioctl.h line 6 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

What's the point of these read/write ioctls? I would expect we need to test read/write interaction with ioctl too, so we would always use them (over these two ioctls)

Purely for testing purposes. The more IOCTLs we have, the more we can test. In particular, these read/write IOCTLs are good because we can test:

• How in/out logic works in Gramine's IOCTL emulation
• How buffer-copy logic works in Gramine's IOCTL emulation

I like these IOCTLs. If you insist, we could replace them with some other IOCTL, but with similar characteristics...

---

gramine-device-testing-module/ioctl.h line 9 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

What's the point of updating this? Isn't always copied just added to it?

But we want to test different logic. It's not like I'm developing a real, reasonable driver. I just want a set of IOCTLs that test as much logic as possible. Testing in/out logic is one important piece, so I decided to add it here.

---

gramine-device-testing-module/ioctl.h line 16 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

ditto

ditto

---

gramine-device-testing-module/ioctl.h line 24 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why this is here then? Why not have struct gramine_ioctl_replace_list __user* next in list struct? See the comment there.

Done.

---

gramine-device-testing-module/ioctl.h line 36 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why not like this?

Done. Fair enough, it's definitely more readable.

---

gramine-device-testing-module/ioctl.h line 38 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Out of curiosity, why 0x33? :)

This is unused, see https://01.org/linuxgraphics/gfx-docs/drm/ioctl/ioctl-number.html. Also, I've just seen it used in other example drivers.

[boryspoplawski]

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)

---

gramine-device-testing-module/main.c line 37 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done. Wanted to make it very explicit that we replace only until the end of the string, but you're right, we can just rely on data->size.

Actually, this could probably work on any kind of binary data (not strings), so maybe we should update description?

---

gramine-device-testing-module/main.c line 146 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

Oops, I've missed we pass off inside ioctl struct, so this is not a sequential write, so it should not update the file offset. I think I just got confused because we do write using ioctl...
Anyway, sorry for the fuss, please delete this line

---

gramine-device-testing-module/main.c line 162 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

ditto

---

gramine-device-testing-module/main.c line 187 at r2 (raw file):

        }
        case GRAMINE_TEST_DEV_IOCTL_REPLACE_LIST: {
            struct gramine_test_dev_ioctl_replace_list list_item;

Please move into do while scope block

---

gramine-device-testing-module/main.c line 194 at r2 (raw file):

                }
                replace_all_occurences(data, list_item.replacement.src, list_item.replacement.dst);
                list_item_user = list_item.next;

In such places you would usually have some loop limit. Do we care enough? Since this is just a test I don't insist.

---

gramine-device-testing-module/ioctl.h line 6 at r1 (raw file):

The more IOCTLs we have, the more we can test

That's not really true, if they bring no new functionality (not saying it was the case here, just in general)

I like these IOCTLs. If you insist, we could replace them with some other IOCTL, but with similar characteristics...

I don't like them because we want to issue normal read/write as well, so this gets weird (doing read and ioctl doing the same...)

Maybe some splice/readv like ioctl? E.g. to read/write from/into struct iovec.
Otoh this would be not much different from what we have now. I don't know anymore, unblocking, I'll leave the decision to you.

---

gramine-device-testing-module/ioctl.h line 38 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

This is unused, see https://01.org/linuxgraphics/gfx-docs/drm/ioctl/ioctl-number.html. Also, I've just seen it used in other example drivers.

1. 19 October 1999

2. '3' 00-1F linux/suspend_ioctls.h, kernel/power/user.c conflict!

So it is used.

[dimakuv]

Reviewable status: 2 of 4 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)

---

gramine-device-testing-module/main.c line 37 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Actually, this could probably work on any kind of binary data (not strings), so maybe we should update description?

Which description? Do you mean the phrase simple string manipulations in the commit message?

---

gramine-device-testing-module/main.c line 146 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Oops, I've missed we pass off inside ioctl struct, so this is not a sequential write, so it should not update the file offset. I think I just got confused because we do write using ioctl...
Anyway, sorry for the fuss, please delete this line

Done.

---

gramine-device-testing-module/main.c line 162 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

ditto

Done.

---

gramine-device-testing-module/main.c line 187 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Please move into do while scope block

Can't do it: main.c:193:17: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]

---

gramine-device-testing-module/main.c line 194 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

In such places you would usually have some loop limit. Do we care enough? Since this is just a test I don't insist.

Done.

---

gramine-device-testing-module/ioctl.h line 6 at r1 (raw file):

Maybe some splice/readv like ioctl? E.g. to read/write from/into struct iovec.

But won't these IOCTLs then conflict with normal readv/writev syscalls? So we'll have the same "what's the point of duplicating syscalls with ioctls"...

Anyway, I prefer to keep these read/write IOCTLs.

---

gramine-device-testing-module/ioctl.h line 38 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

1. 19 October 1999

2. '3' 00-1F linux/suspend_ioctls.h, kernel/power/user.c conflict!

So it is used.

Oops. Changed to 0x81.

The 1999 is misleading -- this file is actually periodically updated. See e.g. https://github.com/torvalds/linux/commits/e229b429bb4af24d9828758c0c851bb6a4169400/Documentation/userspace-api/ioctl/ioctl-number.rst

[boryspoplawski]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)

---

gramine-device-testing-module/main.c line 37 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Which description? Do you mean the phrase simple string manipulations in the commit message?

I meant README.rst

---

gramine-device-testing-module/main.c line 187 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Can't do it: main.c:193:17: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]

Wait, this is a new scope/block, it should work if you put this at the beginning... It works in my local tests.

---

gramine-device-testing-module/ioctl.h line 6 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Maybe some splice/readv like ioctl? E.g. to read/write from/into struct iovec.

But won't these IOCTLs then conflict with normal readv/writev syscalls? So we'll have the same "what's the point of duplicating syscalls with ioctls"...

Anyway, I prefer to keep these read/write IOCTLs.

But we don't implement readv/writev atm

---

gramine-device-testing-module/ioctl.h line 38 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Oops. Changed to 0x81.

The 1999 is misleading -- this file is actually periodically updated. See e.g. https://github.com/torvalds/linux/commits/e229b429bb4af24d9828758c0c851bb6a4169400/Documentation/userspace-api/ioctl/ioctl-number.rst

0x81 will conflict with something as well. You don't need to worry about it, because it doesn't really matter. I would just use something more meaningful, like 'G', but I don't really care.

[dimakuv]

Reviewable status: 2 of 4 files reviewed, 6 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)

---

-- commits line 4 at r4:
simple string manipulations -> simple manipulations on the data blob

---

gramine-device-testing-module/main.c line 37 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

I meant README.rst

Done

---

gramine-device-testing-module/main.c line 187 at r2 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Wait, this is a new scope/block, it should work if you put this at the beginning... It works in my local tests.

Done. Strange, I guess I made a blooper somewhere. It indeed works.

---

gramine-device-testing-module/README.rst line 14 at r4 (raw file):

This driver implements a custom character device, which allows for simple data
blob manipulations. Each open file handle is associated with a context holding

Not sure if data blob is a good phrase. But couldn't come up with something better.

---

gramine-device-testing-module/ioctl.h line 6 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

But we don't implement readv/writev atm

Ah, right, I get it now. I'm still too lazy to do it.

---

gramine-device-testing-module/ioctl.h line 38 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

0x81 will conflict with something as well. You don't need to worry about it, because it doesn't really matter. I would just use something more meaningful, like 'G', but I don't really care.

Well, at least it doesn't conflict with anything well-known currently. And 'G' has something already, so I don't know, I wouldn't want to use it. I'll keep 0x81.

[boryspoplawski]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)

---

gramine-device-testing-module/README.rst line 14 at r4 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Not sure if data blob is a good phrase. But couldn't come up with something better.

Sounds good to me. byte(s) array is another candidate.

[dimakuv]

Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)

---

-- commits line 4 at r4:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

simple string manipulations -> simple manipulations on the data blob

Actually, to simple manipulations on the byte array

---

gramine-device-testing-module/main.c line 130 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I'm postponing this until you are happy with the other comments -- otherwise it will be hard to review.

@boryspoplawski Can I move the code into dedicated functions now?

---

gramine-device-testing-module/README.rst line 14 at r4 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Sounds good to me. byte(s) array is another candidate.

I like byte array. Let me change to this.

[boryspoplawski]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)

---

gramine-device-testing-module/main.c line 130 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

@boryspoplawski Can I move the code into dedicated functions now?

Yes

[dimakuv]

Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)

---

gramine-device-testing-module/main.c line 130 at r1 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Yes

Done.

[boryspoplawski]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)

[dimakuv]

Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @boryspoplawski)

---

-- commits line 2 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Will do during the final rebase. Blocked myself.

Done

---

-- commits line 11 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

All these should be renamed to GRAMINE_TEST_DEV_IOCTL_...

Done

---

-- commits line 4 at r4:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Actually, to simple manipulations on the byte array

Done

[boryspoplawski]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved