add direction per-field support in struct by necipfazil · Pull Request #2008 · google/syzkaller

necipfazil · 2020-08-04T20:55:15Z

Implementation
Testing
Documentation
Using per-field directions in the existing descriptions (this will be contd in a seperate PR)
- Do this change, check erroneous locations for possible utilization of per-field dir
- Check opt fields (sublist is obtained grepping [opt] in sys/linux)
  - Complete: dev_dri, dev_infiniband_rdma, filesystem, fs_ioctl_fscrypt, io_uring, key, perf, sys
  - Incomplete / needs further attention: bpf, dev_video4linux, socket_can, socket_netlink_generic_batadv, socket_netlink_generic_team, socket_netlink_generic_wireguard, socket_netlink_route, socket_netlink_sock_diag, socket_netlink_xfrm, socket, socket_vnet, socket_xdp

dvyukov · 2020-08-05T13:49:47Z

Don't do this is past of this change. If you sure this is the right thing to do, send a separate PR for this. I don't want to think of all potential implications of this additional change in the context of this PR.

Okay, I took this change back (here).

The errors rise after doing this change seem very useful to pinpoint the locations where we can utilize per field directions.

I will think more about other implications of this change, and send another PR to further discuss as you suggested.

The errors rise after doing this change seem very useful to pinpoint the locations where we can utilize per field directions.

That's interesting. If you can find more places to apply per-field attributes, that's definitely useful.
I suspect it may also be possible to find some places to apply the attribute by searching for [opt] resources.

necipfazil · 2020-08-05T15:32:02Z

I try to avoid push forces to make reviewing easier. I will simplify the commit history possibly by squashing later when the PR is ready to merge.

necipfazil · 2020-08-06T16:24:24Z

Do we want to mark a non-resource field with direction attributes?

I did for some in the newly added changes but I can see no use for it, will possibly remove.

On the contrary, it might be even better to let the parent node indicate the direction for the unmarked fields since a struct can be used for different purposes on different syscalls.

By the way, I can move the changes on the descriptions to another PR as it seems to complicate the reviews for the other parts.

dvyukov · 2020-08-07T09:12:13Z

Do we want to mark a non-resource field with direction attributes?

I did for some in the newly added changes but I can see no use for it, will possibly remove.

On the contrary, it might be even better to let the parent node indicate the direction for the unmarked fields since a struct can be used for different purposes on different syscalls.

By the way, I can move the changes on the descriptions to another PR as it seems to complicate the reviews for the other parts.

Direction is still useful for ints b/c output integers are not generated/mutated and input integers are generated/mutated.

necipfazil · 2020-08-10T15:05:42Z

This should be ready for review.

Some changes were done on the existing descriptions to utilize per-field directions.

To ease the review, I will keep doing that in a seperate PR.

codecov · 2020-08-10T15:16:13Z

Codecov Report

Merging #2008 into master will decrease coverage by 0.0%.
The diff coverage is 96.6%.

Impacted Files	Coverage Δ
pkg/ast/ast.go	`100.0% <ø> (ø)`
pkg/compiler/attrs.go	`94.4% <ø> (ø)`
prog/validation.go	`40.9% <66.7%> (ø)`
pkg/compiler/check.go	`93.6% <86.7%> (-0.1%)`	⬇️
pkg/ast/clone.go	`100.0% <100.0%> (ø)`
pkg/ast/format.go	`93.5% <100.0%> (ø)`
pkg/ast/parser.go	`91.0% <100.0%> (+0.1%)`	⬆️
pkg/ast/walk.go	`100.0% <100.0%> (ø)`
pkg/compiler/gen.go	`94.2% <100.0%> (+0.2%)`	⬆️
prog/encoding.go	`85.0% <100.0%> (+0.1%)`	⬆️
... and 8 more

dvyukov · 2020-08-10T15:41:23Z

Also update https://github.com/google/syzkaller/blob/master/docs/syscall_descriptions.md#resources-for-syscall-ordering

dvyukov · 2020-08-12T10:16:45Z

Will be less logic duplication with:

fldDir, fldHasDir := comp.genFieldDir(fld) if !fldHasDir { fldDir = dir } comp.checkTypeCtors(fld.Type, fldDir, false, ctors, inputs, checked)

This is right, done.

dvyukov · 2020-08-12T10:17:46Z

Please format it as:

Direction: dir, }

dvyukov · 2020-08-12T10:25:19Z

I don't fully understand what this TODO is about. Either extend it or remove.

Note: that pointers are always "in" because we need to generate the actual address, even if the pointee is out.

The pointee needed some changes. This is now solved (here).

dvyukov · 2020-08-12T10:30:18Z

Looking at this I am thinking what practice of using the direction we should establish.
There are basic primitives and capabilities, but I think we also need some practice and patterns of how to use the direction in typical situations.
We still have the direction on the pointer, in this case: ptr[inout, bpf_task_fd_query]. If we specify directions on all fields, what direction do we use on the pointer? Or do we use the pointer direction as "default" and don't specify direction for all fields?
Also I think instead of const[0, int32] (out), we should say int32 (out) because if it's out there is no point in adding more details wrt const/0.

If we specify directions on all fields, what direction do we use on the pointer? Or do we use the pointer direction as "default" and don't specify direction for all fields?

If we have a mix of directions in the struct, I just used inout at the ptr level but this isn't necessarily intiuitive to keep as a convention. One other approach might be (which requires some changes in the core code) to omit ptr direction for structs with all fields marked with directions. This would be easier on the eye and allow us to have additional checks at the compile time for such descriptions.

If we have a mix of directions and don't reuse a struct by changing the direction of the pointer, it seemed better to push all the directions to the per-field level for better readability. If the fields of different directions are close in count, which direction do we choose for the ptr, and what could be a good reason for choosing this direction anyway?

Thinking of this more I can't come up with any scheme right now. Let's proceed with your changes. I guess we need to use directions more to gather more experience.

dvyukov · 2020-08-12T10:36:44Z

This looks unrelated. Are you use it's always 0?

The flags field is currently unused and must be zeroed. Different flags to modify the behavior may be added in the future.

source: https://manpages.debian.org/testing/libdrm-dev/drm-gem.7.en.html

The handling of the flags is driver dependent. I didn't see any driver code checking the flags fields though. Thus, converting it back to int32 cause any trouble in the current situation. Let me do that.

If it's not used, it's good to declare it as const[0, int32]. Maybe clarify with a comment:

# Does not seem to be used by any driver. flags const[0, int32]

comment added

dvyukov · 2020-08-12T10:40:44Z

This structure is already passed as in:

ioctl$DRM_IOCTL_RM_MAP(fd fd_dri, cmd const[DRM_IOCTL_RM_MAP], arg ptr[in, drm_map$DRM_IOCTL_RM_MAP])

so it seems there is something wrong here.

Only the handle field is used, which is in.

I just wanted to use per-field direction here to indicate this and to keep it consistent with the other options for drm_map. Removing it shouldn't make any difference since as you said the struct itself is passed as in already.

To avoid confusions, maybe it is best to remove the per-field directions for places where the upper-level ptr direction is enough to cover overall direction.

Another question (for here and other places): if some field is unused (e.g., flags is not used in this drm_map, it is neither checked so any value is valid), it is preferred to make it const[0, T] to avoid having fuzzer mutate values for this, right?

I just wanted to use per-field direction here to indicate this and to keep it consistent with the other options for drm_map.

I see.

To avoid confusions, maybe it is best to remove the per-field directions for places where the upper-level ptr direction is enough to cover overall direction.

Hard to say.
Using ptr direction for the whole struct when it's all in or out is what have in 99% of cases, so I assumed that an explicit direction is used to override the ptr direction.

it is preferred to make it const[0, T]

Yes.

Now the per-field direction is removed and the ptr direction is used as before (and as how it is used all over the descriptions). (here)

dvyukov · 2020-08-12T10:41:17Z

Again, this whole struct is already in:

ioctl$DRM_IOCTL_AGP_FREE(fd fd_dri, cmd const[DRM_IOCTL_AGP_FREE], arg ptr[in, drm_agp_buffer$DRM_IOCTL_AGP_FREE])

As above, this change is now taken back. (here)

dvyukov · 2020-08-12T10:46:44Z

Does this need to be 0? If not, use int32 (out)

google-cla · 2020-08-12T20:49:10Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

necipfazil · 2020-08-12T21:01:31Z

After rebasing this with master, I was trying to drop a few unrelated commits to avoid needing to get the latest kernel and build it.
It resulted in a strange situation for the PR where more people are invited for review.
I removed you (the people invited for review) from the reviewers list but please disregard if you are still getting notifications, and sorry for the ping.

dvyukov reviewed Aug 5, 2020

View reviewed changes

dvyukov reviewed Aug 7, 2020

View reviewed changes

Comment thread pkg/compiler/check.go Outdated

Comment thread prog/types.go Outdated

necipfazil force-pushed the per_field_dir branch 3 times, most recently from 23eb051 to f3980a9 Compare August 10, 2020 15:00

necipfazil marked this pull request as ready for review August 10, 2020 15:04

necipfazil requested review from dvyukov and xairy August 10, 2020 15:04

dvyukov reviewed Aug 12, 2020

View reviewed changes

necipfazil force-pushed the per_field_dir branch from f3980a9 to 444600d Compare August 12, 2020 20:48

necipfazil requested review from blackgnezdo, markjdb, mptre and tuexen as code owners August 12, 2020 20:48

necipfazil removed request for blackgnezdo, markjdb, mptre, tuexen and xairy August 12, 2020 20:49

necipfazil marked this pull request as draft August 12, 2020 20:49

pkg, prog: add per-field direction attribute

94e5b4c

necipfazil force-pushed the per_field_dir branch from 444600d to 30adf4a Compare August 12, 2020 20:52

necipfazil marked this pull request as ready for review August 12, 2020 21:01

necipfazil requested a review from dvyukov August 12, 2020 21:12

sys/linux: use per-field directions in descriptions

ddf9079

necipfazil force-pushed the per_field_dir branch from 30adf4a to ddf9079 Compare August 13, 2020 08:21

dvyukov merged commit cc59e7e into google:master Aug 13, 2020

Conversation

necipfazil commented Aug 4, 2020 • edited by dvyukov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

necipfazil commented Aug 5, 2020

Uh oh!

necipfazil commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dvyukov commented Aug 7, 2020

Uh oh!

Uh oh!

Uh oh!

necipfazil commented Aug 10, 2020

Uh oh!

codecov Bot commented Aug 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

necipfazil Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

necipfazil Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

necipfazil commented Aug 4, 2020 •

edited by dvyukov

Loading

necipfazil commented Aug 6, 2020 •

edited

Loading

codecov Bot commented Aug 10, 2020 •

edited

Loading

necipfazil Aug 12, 2020 •

edited

Loading

necipfazil Aug 12, 2020 •

edited

Loading