/snapd

interfaces: add gpio-memory-control interface #4323

Merged
merged 2 commits into from Nov 30, 2017

Conversation

Reviewers

@zyga zyga

@niemeyer niemeyer

@jdstrand jdstrand

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@kyrofa @zyga @codecov-io @mvo5 @niemeyer @jdstrand
@kyrofa
Member

kyrofa commented Nov 29, 2017

Raspberry Pi systems expose GPIO-only memory at /dev/gpiomem, which provides much faster memory-mapped access than sysfs while being more constrained than /dev/mem.

Support write-access to /dev/gpiomem with a new interface named gpio-memory-control.
@kyrofa
interfaces: add gpio-memory-control interface 
Raspberry Pi systems expose GPIO-only memory at /dev/gpiomem, which
provides much faster memory-mapped access than sysfs while being more
constrained than /dev/mem.

Support write-access to /dev/gpiomem with a new interface named
gpio-memory-control.

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
This commit was signed with a verified signature.
@kyrofa kyrofa Kyle Fazzari
GPG key ID: A7D23FB0E778DEDF Learn about signing commits
1d97112

@kyrofa kyrofa requested a review from jdstrand Nov 29, 2017

@zyga

zyga approved these changes Nov 29, 2017
View changes

Looks good with one question.

interfaces/builtin/gpio_memory_control.go
+const gpioMemoryControlConnectedPlugAppArmor = `
+# Description: Allow writing to /dev/gpiomem on kernels that provide it.
+#
+/dev/gpiomem rw,
@zyga

zyga Nov 29, 2017

Contributor

Don't you need m as well for mmap?

@kyrofa

kyrofa Nov 29, 2017

Member

Honestly I don't know. This worked for my purposes, which given my rudimentary understanding of the lib I'm using uses mmap, but it's possible they're falling back (note that I see no denials, however).

Note also that physical-memory-control does not use m for /dev/mem: https://github.com/snapcore/snapd/blob/master/interfaces/builtin/physical_memory_control.go#L41 .

@jdstrand

jdstrand Nov 29, 2017

Contributor

'm' is only needed when using mmap() with PROT_EXEC.

@zyga
Contributor

zyga commented Nov 29, 2017

@kyrofa once this gets a review from @jdstrand we should work with @sergiocazzolato on a spread test to make sure, you know, that it really works ;-)
@kyrofa
Member

kyrofa commented Nov 29, 2017

[...] we should work with @sergiocazzolato on a spread test to make sure, you know, that it really works ;-)

Do you have rpis in your test rig?
@kyrofa
Member

kyrofa commented Nov 29, 2017 •
Edited 1 time

Huh. Not sure what this Travis failure means:

2017-11-29 20:27:08 Error executing linode:ubuntu-16.04-64:tests/unit/go : 

-----

+ mkdir -p /tmp/static-unit-tests/src/github.com/snapcore

+ cp -ar /home/gopath/src/github.com/snapcore/snapd /tmp/static-unit-tests/src/github.com/snapcore

+ chown -R test:12345 /tmp/static-unit-tests

+ rm -r /tmp/static-unit-tests/src/github.com/snapcore/snapd/vendor/github.com/ /tmp/static-unit-tests/src/github.com/snapcore/snapd/vendor/golang.org/ /tmp/static-unit-tests/src/github.com/snapcore/snapd/vendor/gopkg.in/

+ rm -rf /tmp/static-unit-tests/src/github.com/snapcore/snapd/cmd/autom4te.cache /tmp/static-unit-tests/src/github.com/snapcore/snapd/cmd/configure /tmp/static-unit-tests/src/github.com/snapcore/snapd/cmd/test-driver /tmp/static-unit-tests/src/github.com/snapcore/snapd/cmd/config.status /tmp/static-unit-tests/src/github.com/snapcore/snapd/cmd/config.guess /tmp/static-unit-tests/src/github.com/snapcore/snapd/cmd/config.sub /tmp/static-unit-tests/src/github.com/snapcore/snapd/cmd/config.h.in /tmp/static-unit-tests/src/github.com/snapcore/snapd/cmd/compile /tmp/static-unit-tests/src/github.com/snapcore/snapd/cmd/install-sh /tmp/static-unit-tests/src/github.com/snapcore/snapd/cmd/depcomp /tmp/static-unit-tests/src/github.com/snapcore/snapd/cmd/build /tmp/static-unit-tests/src/github.com/snapcore/snapd/cmd/missing /tmp/static-unit-tests/src/github.com/snapcore/snapd/cmd/aclocal.m4 /tmp/static-unit-tests/src/github.com/snapcore/snapd/cmd/Makefile /tmp/static-unit-tests/src/github.com/snapcore/snapd/cmd/Makefile.in

+ su -l -c 'cd /tmp/static-unit-tests/src/github.com/snapcore/snapd && PATH=/home/gopath/bin:/snap/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin:/usr/lib/go-1.6/bin:/var/lib/snapd/snap/bin GOPATH=/tmp/static-unit-tests ./run-checks --static' test

Obtaining dependencies

Checking docs

Checking formatting

Running vet

Check for usages of http.Status*

Checking spelling errors

Checking for ineffective assignments

Checking for naked returns

Checking all interfaces have minimal spread test

fatal: Not a git repository (or any of the parent directories): .git

All good, what could possibly go wrong.

+ su -l -c 'cd /tmp/static-unit-tests/src/github.com/snapcore/snapd &&     TRAVIS_BUILD_NUMBER=18561     TRAVIS_BRANCH=master     TRAVIS_COMMIT=0235d5feb7409062e29560cff0dd7e69cf16b986     TRAVIS_JOB_NUMBER=18561.1     TRAVIS_PULL_REQUEST=4323     TRAVIS_JOB_ID=309151920     TRAVIS_REPO_SLUG=snapcore/snapd     TRAVIS_TAG=     PATH=/home/gopath/bin:/snap/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin:/usr/lib/go-1.6/bin:/var/lib/snapd/snap/bin     COVERMODE=     TRAVIS=true     CI=true     GOPATH=/tmp/static-unit-tests     ./run-checks --unit' test
@jdstrand

jdstrand requested changes Nov 29, 2017
View changes

Looks good. Please address the review comments and get signoff on the name from @niemeyer.

interfaces/builtin/gpio_memory_control.go
+
+package builtin
+
+const gpioMemoryControlSummary = `allows write access to all gpio memory`
@jdstrand

jdstrand Nov 29, 2017

Contributor

Can you add a comment above this:

// https://github.com/raspberrypi/linux/blob/rpi-4.4.y/drivers/char/broadcom/bcm2835-gpiomem.c
@kyrofa

kyrofa Nov 29, 2017 •
Edited 1 time

Member

Definitely-- done.

interfaces/builtin/gpio_memory_control.go
+`
+
+const gpioMemoryControlConnectedPlugAppArmor = `
+# Description: Allow writing to /dev/gpiomem on kernels that provide it.
@jdstrand

jdstrand Nov 29, 2017

Contributor

Can you update this to be:

# Description: Allow writing to /dev/gpiomem on kernels that provide it (eg,
# via the bcm2835-gpiomem kernel module). This allows direct access to the
# physical memory for GPIO devices (ie, a subset of /dev/mem) and therefore
# grants access to all GPIO devices on the system.
@kyrofa

kyrofa Nov 29, 2017

Member

Sure thing, done.

interfaces/builtin/gpio_memory_control.go
+const gpioMemoryControlConnectedPlugAppArmor = `
+# Description: Allow writing to /dev/gpiomem on kernels that provide it.
+#
+/dev/gpiomem rw,
@zyga

zyga Nov 29, 2017

Contributor

Don't you need m as well for mmap?

@kyrofa

kyrofa Nov 29, 2017

Member

Honestly I don't know. This worked for my purposes, which given my rudimentary understanding of the lib I'm using uses mmap, but it's possible they're falling back (note that I see no denials, however).

Note also that physical-memory-control does not use m for /dev/mem: https://github.com/snapcore/snapd/blob/master/interfaces/builtin/physical_memory_control.go#L41 .

@jdstrand

jdstrand Nov 29, 2017

Contributor

'm' is only needed when using mmap() with PROT_EXEC.

interfaces/builtin/gpio_memory_control.go
+
+func init() {
+ registerIface(&commonInterface{
+ name: "gpio-memory-control",
@jdstrand

jdstrand Nov 29, 2017

Contributor

I'm conflicted on the name. On the one hand /dev/gpiomem gives precisely access to all GPIO memory so gpio-memory-control makes a lot of sense. However, there are several GPIO kernel systems and I wonder if it makes sense to have this interface reflect that this is for one, /dev/gpiomem, and therefore the interface could be named gpiomem-control. I think I prefer what you have, gpio-memory-control, but please get signoff from @niemeyer on the name.

@kyrofa

kyrofa Nov 29, 2017

Member

I am similarly conflicted, and have no strong opinions.

tests/lib/snaps/test-snapd-policy-app-consumer/meta/snap.yaml
@@ -83,6 +83,9 @@ apps:
fwupd:
command: bin/run
plugs: [ fwupd ]
+ gpio-memory-control:
+ command: bin/run
+ plugs: [ gpio-memory-control ]
@jdstrand

jdstrand Nov 29, 2017

Contributor

\o/ missing_interface_spread_test() in run-checks did its job! :)

@kyrofa

kyrofa Nov 29, 2017

Member

Yes, I actually said "gee, thanks" out loud!

@zyga

zyga Nov 29, 2017

Contributor

Wow, I didn't know we had that. Thank you!!!

@jdstrand jdstrand requested a review from niemeyer Nov 29, 2017

@kyrofa
Add more explicit comments 
Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
This commit was signed with a verified signature.
@kyrofa kyrofa Kyle Fazzari
GPG key ID: A7D23FB0E778DEDF Learn about signing commits
fbda082
@codecov-io

codecov-io commented Nov 29, 2017 •
Edited 2 times

Codecov Report

❗️ No coverage uploaded for pull request base (master@804514c). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #4323   +/-   ##
========================================
  Coverage          ?   76.2%           
========================================
  Files             ?     446           
  Lines             ?   38784           
  Branches          ?       0           
========================================
  Hits              ?   29555           
  Misses            ?    7215           
  Partials          ?    2014
Impacted Files Coverage Δ
interfaces/builtin/gpio_memory_control.go 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 804514c...fbda082. Read the comment docs.
@mvo5
Collaborator

mvo5 commented Nov 30, 2017

Looks good, thanks for adding this interface! This can go in once we have sign-off for the name from @niemeyer
@zyga
Contributor

zyga commented Nov 30, 2017

@kyrofa and you get to write a forum post about it and we get to link it via interface meta-data in snapd :)
@niemeyer

niemeyer approved these changes Nov 30, 2017
View changes

LGTM, name looks good too, thanks!

@kyrofa kyrofa merged commit 8ecec65 into snapcore:master Nov 30, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kyrofa kyrofa deleted the kyrofa:feature/gpiomem_interface branch Nov 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment