interfaces/builtin: use udev tagging more broadly #3617

adglkh · Jul 25, 2017

It's a long-standing bug in snapd.
https://bugs.launchpad.net/snapd/+bug/1675738

We have some interfaces using udev tagging, but quite a few interfaces are still without udev tagging. So when people create a snap which has a combination of udev tagging plugs and none udev tagging plugs, an issue occurs due to device access denied since snap-confine only loads udev rules and creates a device cgroup for the tagged devices, excluding untagged ones.

mvo5 · Jul 27, 2017

mvo5 reviewed Jul 27, 2017

View changes

A bit of a meta-question - would it make sense to add an optional connectedPlugUdev to commonInterface so that not all the interfaces that use commonInterface need to be exapnded ?

Yes, it makes sense for some interfaces.
I'll fix it in the next commit.
Thanks.

jdstrand · Jul 27, 2017

@adglkh - there are a lot of conflicts now. Can you clean these up? Perhaps this has something to do with @zyga's most recent commits?

codecov-io · Jul 28, 2017

Codecov Report

Merging #3617 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #3617      +/-   ##
=========================================
+ Coverage   75.63%   75.7%   +0.06%     
=========================================
  Files         407     407              
  Lines       35034   35093      +59     
=========================================
+ Hits        26499   26566      +67     
+ Misses       6654    6648       -6     
+ Partials     1881    1879       -2

Impacted Files	Coverage Δ
interfaces/builtin/time_control.go	`100% <ø> (ø)`	⬆️
interfaces/builtin/uhid.go	`100% <ø> (ø)`	⬆️
interfaces/builtin/kvm.go	`100% <ø> (ø)`	⬆️
interfaces/builtin/hardware_random_observe.go	`100% <ø> (ø)`	⬆️
interfaces/builtin/broadcom_asic_control.go	`100% <ø> (ø)`	⬆️
interfaces/builtin/physical_memory_observe.go	`100% <ø> (ø)`	⬆️
interfaces/builtin/physical_memory_control.go	`100% <ø> (ø)`	⬆️
interfaces/builtin/hardware_random_control.go	`100% <ø> (ø)`	⬆️
interfaces/builtin/io_ports_control.go	`100% <ø> (ø)`	⬆️
interfaces/builtin/udisks2.go	`100% <100%> (ø)`	⬆️
... and 26 more

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 55c86cb...4dc3c47. Read the comment docs.

jdstrand · Aug 1, 2017

@adglkh - unfortunately there are more conflicts. Can you clean these up (again)?

morphis · Aug 2, 2017

morphis reviewed Aug 2, 2017

View changes

Should be 2016-2017

Thanks.
I'll fix it accordingly in each *_test.go I modified.

jdstrand · Aug 2, 2017

jdstrand requested changes Aug 2, 2017

View changes

This is very much on the right track, thanks! Please make the requested changes.

Curious on the testing performed-- I know you updated the testsuite but did you blackbox test by connecting these plugs and seeing if you could access the devices?

Where did you get this list? I don't think it is quite right be looking at my system and reading http://www.reactivated.net/writing_udev_rules.html#strmatch. Also, I don't think we need 'NAME' here since we are only tagging kernel devices and not trying to rename them (this is true everywhere you use NAME)-- did you add NAME for a specific reason?

I think you want this instead:

KERNEL=="controlC[0-9]*",        TAG+="%[1]s"
KERNEL=="hwC[0-9]*D[0-9]*",      TAG+="%[1]s"
KERNEL=="pcmC[0-9]*D[0-9]*[cp]", TAG+="%[1]s"
KERNEL=="midiC[0-9]*D[0-9]*",    TAG+="%[1]s"
KERNEL=="timer",                 TAG+="%[1]s"
KERNEL=="seq",                   TAG+="%[1]s"

Not to mention, I feel like the list may be incomplete. I see in /lib/udev/rules.d references to card*. I think at least this should be added:

SUBSYSTEM=="sound", KERNEL=="card[0-9]*", TAG+="%[1]s"

Adding the name here ensures that the sound device node appears exclusively in the /dev/snd directory where they are created in my system and also it meant to be consistent with /dev/snd/* in apparmor policy for alsa interface. Removing the name results in sound devices are created under /dev directory by default. But I couldn't find them there(/dev) in my system.

Basically, it's harmless to have 'NAME' here for each device. I need to confirm this at this point before I proceed.

SUBSYSTEM=="sound", KERNEL=="card[0-9]*", TAG+="%[1]s"
Yes, I saw it. Thanks for the mention.

Well, the system should already have rules for putting devices in /dev/snd. Your rules are only about tagging the devices. AIUI the NAME field, it does affect where things are put so I didn't want to override what the system is already providing so suggested that NAME be removed.

Are you saying that if you omit NAME, when you connect the interface you now see /dev/pcm..., etc? If that is the case, we should look at why that is happening-- we only want to add a tag, not change existing rules.

Okay, got your point.
Let's remove NAME key for each rule of the interfaces.
I'll also give it a try on snap to check if it works for me with the fine-tuned udev rules.
Thanks.

It's a little weird that the expectedSnippet declaration is so far from its use. Can you move it after the apparmor tests and before the udev tests? This is true for other _test.go files as well.

Sure.

hci* are virtual devices-- do virtual devices need to be added to the device cgroup?. Also, bluetooth-control does have an apparmor access for /dev/vhci but there is no udev rule for it.

I thought this was going to be (to match the apparmor /dev access):

SUBSYSTEM=="net", KERNEL=="linux-user-bde", TAG+="%[1]s"
SUBSYSTEM=="net", KERNEL=="linux-kernel-bde", TAG+="%[1]s"
SUBSYSTEM=="net", KERNEL=="linux-bcm-knet", TAG+="%[1]s"

where did you get your rule?

Here is the udev rule from upstream,
https://git.openswitch.net/cgit/openswitch/ops-build/commit/?id=c0e324d0e76e9464f115434cce74301b5794ff61

I'm afraid it doesn't work that way if we use kernel module name rather than the 'bdev' (bcm device) in this case.

So long as we are using base_dev_name=bdev in the module loading, I guess what you had before would work. Where do we guarantee that is happening?

If using 'bdev' instead, can you add a comment above your rule?

Yes, basically, this interface was added since it's needed by a customer project.
I compared the rule with what was being added into upstream and found the difference here.
I'll do a further investigation on these two rules and confirm the final behavior on real device and will add the upstream link above the rule if it's the one we needed.

@jdstrand, I took a closer look today on a machine a kernel built for openswitch.

A udev rule(/etc/udev/rules.d/60-bcm-driver.rules) executes external comment to creates three character device files(via mknod) by using loadable kernel modules, However, I am not able to query the information for all three character devices on the machine.
http://paste.ubuntu.com/25262959/

I found some hints after exporting the content of the udev database (keyword: "linux-kernel-bde" and "bcm0")
http://paste.ubuntu.com/25262850/
Then query all information for the first matched device
http://paste.ubuntu.com/25262846/
Meanwhile, there's another 'bcm0' device can be found in sysfs (the second matched device)
http://paste.ubuntu.com/25262841/

Based on these findings, the udev tagging rule for this interface would be

SUBSYSTEM=="pci", DRIVER=="linux-kernel-bde", TAG+="%[1]s"
SUBSYSTEM=="net", KERNEL=="bcm[0-9]*", TAG+="%[1]s"

Also apparmor policy is needed to update somehow by adding a few lines, here are what I have in mind.

  /sys/devices/pci[0-9]*/**/config r,
  /sys/devices/pci[0-9]*/**/{,subsystem_}device r,
  /sys/devices/pci[0-9]*/**/{,subsystem_}vendor r,

  /sys/bus/pci/devices/** r,
  /run/udev/data/+pci:[0-9]* r,

+1 on the udev changes. The apparmor policy is too broad. Can you give the specific denials the prompted each rule?

I don't think we want to have udev rules for docker-supprort since docker can be used to add devices to a container. If we add udev rules, the only devices it could add would be those here. Perhaps it makes sense to document in the interface that we are intentionally not creating udev rules.

Yes, docker supports to add a rule to the cgroup allowed devices list.
I'll drop udev rules for docker-support interface in this case and document the reason.
Thanks for the mention.

/dev/tty is already added by defaults (see cmd/snap-confine/udev-support.c)

I will remove it in the next commit.

We have a joystick interface. I wonder if we should omit js here?

Yes, I should remove it to avoid overlapping each other.
Same for /dev/ttyS* in modem_manager.go

Where did you see /dev/input/ts*? Also, instead of mouse*, event* and ts*, I think we want mouse[0-9]*, event[0-9]* and ts[0-9]*. Also, you should drop 'NAME=' (see above).

RE: /dev/input/ts*
1.https://kernel.googlesource.com/pub/scm/linux/hotplug/udev/+/127/rules/rules.d/50-udev-default.rules#27
2.https://github.com/kergoth/tslib
It's a udev rule for the touchscreen device.

Ah, thanks! :)

Would it make sense to move this to common.go like you did UDevConnectedPlug?

Yes, it makes sense.

I think you meant to remove this since you are adding per-command lines via modemManagerPermanentSlotUDevTag.

This includes /dev/ttyS* which might be what you want. Can you comment? If not, this should work:

# omit ttyS* since that is covered by the serial-port interface
KERNEL=="tty[A-RT-Z]*[0-9]|cdc-wdm[0-9]*" TAG+="%s"

Aha, I didn't notice we already have /dev/ttyS* in serial-port interface.

According to https://github.com/torvalds/linux/blob/master/Documentation/admin-guide/devices.txt, I think we should use:

KERNEL=="tap[0-9]*",   TAG+="%[1]s"

We need this here (use of /dev/net/tun creates /dev/tun* devices):

KERNEL=="tun", TAG+="%[1]s"
KERNEL=="tun[0-9]*", TAG+="%[1]s"

Same comment here wrt ttyS*.

and here wrt to tun devices.

We don't need SYMLINK here. We are just tagging.

Drop 'NAME=' here and you missed /dev/vchiq.This should be:

SUBSYSTEM="drm", KERNEL=="card[0-9]*", TAG+="%[1]s"
KERNEL=="nvidia*", TAG+="%[1]s"
KERNEL=="vchiq", TAG+="%[1]s"

Since the apparmor policy has a redundancy too, can you update it to remove /dev/nvidiactl rw, and /dev/nvidia-modeset rw, since /dev/nvidia* rw, covers all of them?

Sure.
One quick question: Is vchiq kept for bcm device(GPU) on PI?
Thanks.

I'm not terribly familiar with this device. I see these:

We can drop NAME and SYMLINK. /dev/scd* aren't always symlinks, so adjust to be:

KERNEL=="sr[0-9]*",  TAG+="%[1]s"
KERNEL=="scd[0-9]*", TAG+="%[1]s"

Thank you for adding these missing tests.

The apparmor rule was /dev/tty[^0-9]* rw,, so this should be:

KERNEL=="tty[A-Z]*", TAG+="%[1]s"

My stupid mistake.

Like above, drop NAME and update the globs to be:

KERNEL=="controlC[0-9]*",        TAG+="%[1]s"
KERNEL=="pcmC[0-9]*D[0-9]*[cp]", TAG+="%[1]s"
KERNEL=="timer",                 TAG+="%[1]s"

This should be:

SUBSYSTEM=="rtc", TAG+="%s"

This should be:

KERNEL=="tpm[0-9]*", TAG+="%s"

I know we're missing vd*. I think perhaps change this to:

SUBSYSTEM=="block", TAG+="%[1]s"
KERNEL=="fd[0-9]*, TAG+="%[1]s"
# This tags all USB devices, so we'll use AppArmor to mediate specific access (eg, /dev/sd* and /dev/mmcblk*)
SUBSYSTEM=="usb", TAG+="%[1]s"

Can you update the apparmor policy to have this:

/dev/vd* r,

My concerns on this line were that it probably opens unnecessary holes in the confinement system since we grant people permission accessing all USB devices. However I forget, on the other hand, we could enforce AppArmor profile constraints for the confinement escape. I guess this strategy applies to raw_usb, broadcom_asic_control too. Thanks for your comments above

@jdstrand I double-checked here
fd[0-9]* would be covered by SUBSYSTEM=="block", do we really need to keep fd[0-9]* in the udev rule for udisk2 interface?

I would be fine to remove the fd lines-- I was looking at the udev rules and saw it handled floppy a little differently, so put it. Really, it doesn't matter-- the apparmor policy currently only allows /dev/sd* and /dev/mmcblk* (and if you added it, /dev/vd*). I think you are right to just use block and usb subsystems and then we'll use apparmor globbing for which to allow.

jdstrand · Aug 2, 2017

jdstrand reviewed Aug 2, 2017

View changes

Thank you for adding this to the testsuite.

jdstrand · Aug 2, 2017

@adglkh - note #3609 which is on the verge of landing. If it lands, it might be nice (though not required for merging) to update it to use UDevConnectedPlug.

adglkh · Aug 3, 2017

Thanks for a detailed review.

I have a bunch of simple apps with individual and multiple interfaces combinations declared in its yaml file for black-box testing(udev tagging verification). I was also thinking if it's good timing to update spread test of snap-confine. We moved snap-confine into snapd, but it looks like we didn't update the doc and scripts accordingly(I encountered the folder&file path issue during running spread-test ). Also in order to run spread test of snap-confine, we're still relying on https://git.launchpad.net/snap-confine to build the package. It failed since one dependency(xfslibs-dev) we recently added is missing in control file of snap-confine repo on lp.

Probably we need a separated PR to fix it.

jdstrand · Aug 3, 2017

@zyga - could you comment on @adglkh's comments regarding spread tests?

zyga · Aug 4, 2017

This branch is on my TODO list but it's somewhat pushed back given how huge it is. I'll try to review it today though.

zyga · Aug 4, 2017

zyga reviewed Aug 4, 2017

View changes

This is a pretty big PR and I'd like to de-compose it somewhat. Here's what I propose:

create a new PR with the new common udev function and (perhaps separate) simplification of existing interfaces to let them become common
tweaks and updates to tests (I'm trying to unify how those are done lately)
per-interface PRs that do udev tagging. This seems painful but I think it is much saner than iterating on one huge PR that touches a thousand lines in half of the interface files.

If you don't disagree I'll start chopping this into pieces and iterating on all the elements.

Thank you for doing this. I wanted to add something like this for a long time lately

zyga · Aug 7, 2017

Yay, the PR shrank by a few lines. I'll iterate on the remaining changes and try to get them merged to master separately so that this PR can focus just on actual udev changes.

adglkh · Aug 15, 2017

Ooops, Looks like sth went wrong with the merging.
Will fix it soon.

adglkh · Aug 15, 2017

This branch now has halved diff size after merging master into it and resolving lots of conflicts as well.
I think we're good to go.
@zyga Could you help review it?
@jdstrand Could you take another look at this PR?
Thanks.

zyga · Aug 16, 2017

zyga reviewed Aug 16, 2017

View changes

I made a few remarks. I don't mind merging this if @jdstrand agrees. I highlighted some parts that could land immediately in separate PRs and one thing that is not related to udev.

Thanks!

s/###SLOT_SECURITY_TAGS###/###CONNECTED_SECURITY_TAGS###/

Thanks!

Nice, finally :-)

Thanks!

Let's please use strings.Replace

I'd love the same replacement logic that we use elsewhere simply because this will lessen the friction to unify everything at some point in the future.

This could be a separate PR (just this part) if you feel like it.

Let's please use strings.Replace here

This could be a separate PR that lands very quickly.

This is not related to the change (not that I dislike it but it confuses the premise of the PR)

This could be a separate PR that lands very quickly.

adglkh · Aug 16, 2017

I just wanna keep the same code patterns in test cases for all interfaces I worked on and squeeze something unrelated to udev tagging changes in this PR.
I should submit a separated PR to do that since it makes more sense. Keeping diff sizes smaller is nice. That'd be helpful for code review as well.
Sorry about that. I'll do it now.

zyga · Aug 17, 2017

@jdstrand can you please re-review this PR? Garry worked on making the diff nice and small by merging other branches into master.

jdstrand · Aug 17, 2017

Yes. I would've done it sooner but @adglkh said he had more to do. I see new commits since then, so I'll take a look today.

jdstrand · Aug 17, 2017

jdstrand requested changes Aug 17, 2017

View changes

This is really close; thanks for all your hard work on this. In addition to the other inline comments, please adjust TAG+="###SLOT_SECURITY_TAGS###" to be TAG+="###CONNECTED_SECURITY_TAGS###". It seems these crept in some previous commits, but lets make sure at least the things we are changing in this PR are correct. You'll want to also change interfaces/builtin/common.go

You changed the copyright in bluetooth_control_test.go, but forgot to update it and bluetooth_control.go for /dev/vhci, which is included in the apparmor policy. Can you add that?

/dev/vhci is just a node that vhci driver created. I am not able to find it in udev database even if it exists. http://paste.ubuntu.com/25336192/
If we add udev rules, the devices we want to tag are the virtual devices.
`SUBSYSTEM=="bluetooth", KERNEL=="hci[0-9]*", TAG+="%s"

Yes, "%s" will be changed to ###CONNECTED_SECURITY_TAGS###

Oh that is interesting, but I really like your idea of SUBSYSTEM bluetooth. How about just treat this like raw-usb and do:

SUBSYSTEM=="bluetooth", TAG+="%s

The 'hci' devices are virtual devices and not yet mediated by AppArmor or devices that need to be added to the device cgroup. However, tagging all bluetooth devices helps future-proof a bit in case /dev/vhci ends up showing up in udev or other bluetooth devices show up in /dev.

Yeah, that's exactly what we're looking for.
Thanks.

The variable name is bluezConnectedPlugUDev but the TAG is ###SLOT_SECURITY_TAGS###, which is not quite right. With apparmor policy there is plug side and slot side policy, but with udev tagging, there is only the tag of what is being connected. I suggest changing to ###CONNECTED_SECURITY_TAGS###

Thanks for fixing all these! :)

This doesn't seem right since I suspect that the devices might show up in other places than /sys/devices/pci0000:00/0000:00:1c.0. For example, I don't have these modules loaded on my laptop, but I do have these paths.

Until https://bugs.launchpad.net/ubuntu/+source/snapd/+bug/1570860/comments/1 is implemented, we probably need to use broader glob rules like:

/sys/devices/pci[0-9]*/**/config r,
/sys/devices/pci[0-9]*/**/{,subsystem_}device r,
/sys/devices/pci[0-9]*/**/{,subsystem_}vendor r,

/sys/bus/pci/devices/ r,
/run/udev/data/+pci:[0-9]* r,

Note, I changed the /sys/bus/pci/devices rule because it should be filled with symlinks, not actual files, and apparmor resolves symlinks.

Ah, right. There was another common detail I didn't take into account.
Using broader glob rules is a good fit, at least for now.
I take a note here.
RE: /sys/bus/pci/devices rule
I'd expect we need the same change for opengl interface.

Thanks for your explanation!

I'm surprised you can use KERNEL=="0000:01:00.0" here, but if it works, +1.

Note, I don't think we should have pastebins in the comments since I think they go away after awhile. Perhaps just mention the importants bits from the pastes (possibly with examples entries instead of all of them).

I'll remove the comments here to keep it simple.

s/###SLOT_SECURITY_TAGS###/###CONNECTED_SECURITY_TAGS###/

Thanks!

s/###SLOT_SECURITY_TAGS###/###CONNECTED_SECURITY_TAGS###/

I think this should be:

KERNEL=="tty[A-RT-Z]*[0-9]*|cdc-wdm[0-9]*", TAG+="%s"

(note the '*' after tty[A-RT-Z]*[0-9]).

That said, in thinking about this more, the modem-manager interface apparmor policy currently allows /dev/ttyS[0-9]* so in retrospect, I think we should use this so as not to break existing snaps that don't plugs serial-port (not to mention, serial-port only shows up with configured gadgets so there would be no way to have modem-manager use /dev/ttyS*, which would be a regression):

KERNEL=="tty[A-Z]*[0-9]*|cdc-wdm[0-9]*", TAG+="%s"

s/###SLOT_SECURITY_TAGS###/###CONNECTED_SECURITY_TAGS###/

Same here as with modem-manager:

KERNEL=="tty[A-Z]*[0-9]*|cdc-wdm[0-9]*", TAG+="###SLOT_SECURITY_TAGS###"

These two ttys are covered by the first rule.

Also, I think we need (for other apparmor accesses):

KERNEL=="modem[0-9]*",       TAG+="###SLOT_SECURITY_TAGS###"
KERNEL=="chnlat[0-9]*",      TAG+="###SLOT_SECURITY_TAGS###"
KERNEL=="rild[0-9]*",        TAG+="###SLOT_SECURITY_TAGS###"

Also, s/###SLOT_SECURITY_TAGS###/###CONNECTED_SECURITY_TAGS###/'

I didn't find above three udev rules after digging around.
RE: KERNEL=="modem[0-9]*"

Basically, Linux modem drivers set up the modem device /dev/modem as a symbolic link to the actual device to /dev/ttyS*. So first entry would cover this case.
And /dev/socket/rild is just a socket created by rild daemon when it runs during system boot up. See here
https://stackoverflow.com/questions/11111067/how-does-modem-code-talk-to-android-code/11111953#11111953

I suspect if it's an actual device node we can find in udev database. Same for chnlat.

So I doubt if they're really needed putting here to tag device node which does not actually exist in system.

If modem is always a symlink, then that's fine (we could remove the apparmor rule then too since apparmor resolves symlinks).

If rild and chnlat are actually a sockets, then we don't need a udev rule. Can you add a comment above ofonoPermanentSlotUDevTag that we intetionally skipped modem, rild and chnlat, and why?

Okay, no problem.
I'll add my comments there.

s/###SLOT_SECURITY_TAGS###/###CONNECTED_SECURITY_TAGS###/

We're using this elsewhere:

KERNEL=="tty[A-Z]*[0-9]*", TAG+="###SLOT_SECURITY_TAGS###"

Also, s/###SLOT_SECURITY_TAGS###/###CONNECTED_SECURITY_TAGS###/

s/###SLOT_SECURITY_TAGS###/###CONNECTED_SECURITY_TAGS###/

jdstrand · Aug 18, 2017

jdstrand approved these changes Aug 18, 2017

View changes

+1. Thanks for working through this large task! This is going to really help people.

Just a request to add one comment and then one question that I'll leave to your discretion to fix.

Can you add a comment here:

# These are broader than they needs to be, but until we query udev
# for specific devices, use a broader glob

Sure. :)

Is KERNEL=="0000:01:00.0" correct (ie, is it too narrow?). If so, perhaps just:

SUBSYSTEM=="pci", DRIVER=="linux-kernel-bde", TAG+="###CONNECTED_SECURITY_TAGS###"

The above rules address my concerns as well.
To some extent, IMHO, The following rule is probably good enough in this case
DRIVER=="linux-kernel-bde", TAG+="###CONNECTED_SECURITY_TAGS###"
Let's finalise with yours proposal
SUBSYSTEM=="pci", DRIVER=="linux-kernel-bde", TAG+="###CONNECTED_SECURITY_TAGS###"

adglkh · Aug 18, 2017

Without your guys' guidance and assistance, it could not happen.
@jdstrand @zyga
Could you help take another look at this PR?
Thanks again.

niemeyer · Aug 23, 2017

niemeyer requested changes Aug 23, 2017

View changes

A couple of questions:

Indeed, that looks way too broad. In fact, a bit too broad to be right. Not sure it makes sense to turn the "broadcoam-asic" interface in a sort of "read about every single PCI device in the system".

@jdstrand Note that we won't be able to change that in the future without the potential of breaking clients unknowingly.

What's the plan here?

I think it's related to hardware assignment. We have a bug on launchpad tracking on this. Not sure where are we with that.
@jdstrand Could you share the updates and plan here?
Thanks

@niemeyer, @adglkh - they are necessarily broad because in PR review it was found there was nothing in the /sys/devices path alone that made it 'broadcom-asic'. I believe it is technically possible for snapd to interrogate the system via udev to get the precise /sys/devices and /run/udev/data paths for the device (see #1226 (comment) for back of the napkin idea), but this work is neither prioritized nor assigned. As a result, this interface along with existing opengl and camera interfaces use rules that are broader than they should be (but we comment in the policy in camera and opengl about the leak). While the accesses in question do constitute an information leak, they are not terribly significant (which is why the work is not prioritized/assigned) and in the 'future work' bucket.

I think the plan should be:

continue with the access as written since without it, things break
add a comment that this is an information leak (see opengl.go)
at some future time, implement the udev querying in snapd and have new interfaces in series 16 use this
in some future iteration of snappy which is generating different policy (eg, something not 'series 16'), migrate existing interfaces that use the broad globs to using only the outputs of udev querying

Has this change been tested? Why was the prior case necessary and why is it not needed anymore now?

This also seems unrelated to device tagging.

We changed this rule because /sys/bus/pci/devices/ should be filled with symlinks, not actual files, and apparmor resolves symlinks. Also see here
https://github.com/adglkh/snapd/blob/1f4a0fe9c464e805afc9ccfe241b96b14d13248b/interfaces/builtin/opengl.go#L48-L50
The prior case matches any file or directory in or under /sys/bus/pci/devices/. It seems to be not needed anymore now

The same for broadcom_asic_control interface.
https://github.com/snapcore/snapd/pull/3617/files/1f4a0fe9c464e805afc9ccfe241b96b14d13248b#diff-5ef61751d67fe46b2e4b8167e20330c8R48

Yes, it's unrelated to udev tagging rule. It makes more sense to fix it in a separated PR.
Sorry about that.

I reverted this change now - please open a new PR with this particular change. We are keen to land this one this is why I did the change :)

Sure, no problem.:)

niemeyer · Aug 25, 2017

niemeyer approved these changes Aug 25, 2017

View changes

LGTM assuming we drop the unrelated opengl change from this PR (okay for another, but let's please test the change in real-world use first). For the other point, LGTM given comments from @jdstrand.

Thanks for the PR, by the way!

mvo5 · Aug 29, 2017

I changed the opengl.go as suggested by Gustavo and de-conflicted.

mvo5 · Aug 29, 2017

This is failing currently in linode:ubuntu-core-16-64:tests/main/install-store:reexec0 which is a new test in master. I suspect this is because this is racy, I'm working on a fix for the race in https://github.com/snapcore/snapd/compare/master...mvo5:nmcli-regression-test?expand=1

mvo5 · Aug 29, 2017

mvo5 reviewed Aug 29, 2017

View changes

I removed this as it is racy in this PR at least. #3825 is a better test

jdstrand · Aug 29, 2017

The changes since my last review approval look fine. +1

adglkh referenced this pull request Jul 25, 2017
Closed
Using udev tagging for snap interfaces #3587

jdstrand deleted a comment from adglkh Aug 3, 2017

adglkh referenced this pull request Aug 16, 2017
Merged
interfaces: convert uhid to common interface and test cases improvement for time_control and opengl #3745

zyga added this to the 2.28 milestone Aug 17, 2017

niemeyer changed the title from Using udev tagging for snap interfaces to interfaces/builtin: use udev tagging more broadly Aug 23, 2017

mvo5 referenced this pull request Aug 29, 2017
Merged
tests: add nmcli regression test #3825

@@ -1,7 +1,7 @@
          // -*- Mode: Go; indent-tabs-mode: t -*-
          /*
-        - * Copyright (C) 2016 Canonical Ltd
+        + * Copyright (C) 2017 Canonical Ltd

+        +KERNEL=="pcm[CD0-9cp]*",  NAME="snd/%%k", TAG+="%[1]s"
+        +KERNEL=="midiC[D0-9]*",   NAME="snd/%%k", TAG+="%[1]s"
+        +KERNEL=="timer",          NAME="snd/%%k", TAG+="%[1]s"
+        +KERNEL=="seq",            NAME="snd/%%k", TAG+="%[1]s"

+        +KERNEL=="midiC[D0-9]*",   NAME="snd/%k", TAG+="snap_other_app"
+        +KERNEL=="timer",          NAME="snd/%k", TAG+="snap_other_app"
+        +KERNEL=="seq",            NAME="snd/%k", TAG+="snap_other_app"
+        +`

+        +# /dev/mapper/docker* rw,
+        +#
+        +# dmsetup tool is required to apply the following udev rule. However it looks like we don't have dmsetup pre-installed on core.
+        +KERNEL=="dm-[0-9]*", PROGRAM="/sbin/dmsetup info -c --noopencount --noheadings -o name -j %%M -m %%m", NAME="%%k", SYMLINK+="mapper/%%c", TAG+="%[1]s"

+        +KERNEL=="mice",   NAME="input/%%k", TAG+="%[1]s"
+        +KERNEL=="mouse*", NAME="input/%%k", TAG+="%[1]s"
+        +KERNEL=="event*", NAME="input/%%k", TAG+="%[1]s"
+        +KERNEL=="js*",    NAME="input/%%k", TAG+="%[1]s"

		func init() {
		- registerIface(&commonInterface{

snapcore/snapd

Join GitHub today

interfaces/builtin: use udev tagging more broadly #3617

Conversation

adglkh commented Jul 25, 2017

adglkh added some commits Jul 12, 2017

All checks have failed

All checks have failed

All checks have failed

Some checks were not successful

Some checks were not successful

All checks have passed

adglkh referenced this pull request Jul 25, 2017

mvo5 reviewed Jul 27, 2017 View changes

adglkh added some commits Jul 27, 2017

All checks have failed

All checks have failed

jdstrand commented Jul 27, 2017

All checks have passed

codecov-io commented Jul 28, 2017 • edited Edited 1 time codecov-io Aug 2, 2017

Codecov Report

jdstrand commented Aug 1, 2017

adglkh added some commits Aug 2, 2017

Some checks were not successful

All checks have passed

morphis reviewed Aug 2, 2017 View changes

jdstrand requested changes Aug 2, 2017 View changes

adglkh Aug 3, 2017 • edited Edited 1 time adglkh Aug 3, 2017

jdstrand reviewed Aug 2, 2017 View changes

jdstrand commented Aug 2, 2017

adglkh commented Aug 3, 2017

jdstrand deleted a comment from adglkh Aug 3, 2017

jdstrand commented Aug 3, 2017

zyga commented Aug 4, 2017

zyga reviewed Aug 4, 2017 View changes

adglkh and others added some commits Aug 4, 2017

Some checks were not successful

Some checks were not successful

zyga commented Aug 7, 2017

adglkh commented Aug 15, 2017

adglkh added some commits Aug 15, 2017

Some checks were not successful

Some checks were not successful

adglkh commented Aug 15, 2017

zyga reviewed Aug 16, 2017 View changes

adglkh commented Aug 16, 2017

adglkh added a commit to adglkh/snapd that referenced this pull request Aug 16, 2017

adglkh referenced this pull request Aug 16, 2017

adglkh added some commits Aug 16, 2017

Some checks were not successful

zyga commented Aug 17, 2017

zyga added this to the 2.28 milestone Aug 17, 2017

jdstrand commented Aug 17, 2017

jdstrand requested changes Aug 17, 2017 View changes

adglkh Aug 18, 2017 • edited Edited 1 time adglkh Aug 18, 2017

jdstrand Aug 18, 2017 • edited Edited 1 time jdstrand Aug 18, 2017

mvo5 reviewed Jul 27, 2017

View changes

codecov-io commented Jul 28, 2017 •

Edited 1 time

codecov-io
Aug 2, 2017

morphis reviewed Aug 2, 2017

View changes

jdstrand requested changes Aug 2, 2017

View changes

adglkh Aug 3, 2017 •

Edited 1 time

adglkh
Aug 3, 2017

jdstrand reviewed Aug 2, 2017

View changes

zyga reviewed Aug 4, 2017

View changes

zyga reviewed Aug 16, 2017

View changes

jdstrand requested changes Aug 17, 2017

View changes

adglkh Aug 18, 2017 •

Edited 1 time

adglkh
Aug 18, 2017

jdstrand Aug 18, 2017 •

Edited 1 time

jdstrand
Aug 18, 2017