| Message ID | 20230313175047.20719-1-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | flask: label-pci: Allow specifying optional irq label | expand |
On 3/13/23 13:50, Jason Andryuk wrote: > IRQs can be shared between devices, so using the same label as the PCI > device can create conflicts where the IRQ is labeled with one of the > device labels preventing assignment of the second device to the second > domain. Add the ability to specify an irq label distinct from the PCI > device, so a shared irq label can be specified. The policy would then > be written such that the two domains can each use the shared IRQ type in > addition to their labeled PCI device. That way we can still label most > of the PCI device resources and assign devices in the face of shared > IRQs. > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com> > Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com> > --- > v2: > Describe usage in docs/misc/xsm-flask.txt > --- > docs/misc/xsm-flask.txt | 16 ++++++++++++++++ > tools/flask/utils/label-pci.c | 13 ++++++++++--- > 2 files changed, 26 insertions(+), 3 deletions(-) > > diff --git a/docs/misc/xsm-flask.txt b/docs/misc/xsm-flask.txt > index 2419c5cf29..ba89ebbfd8 100644 > --- a/docs/misc/xsm-flask.txt > +++ b/docs/misc/xsm-flask.txt > @@ -205,6 +205,22 @@ parameter, which can also be changed using xl setenforce). When using the > default types for domains (domU_t), the example policy shipped with Xen should > allow the same operations on or between domains as when not using FLASK. > > +By default, flask-label-pci labels the device, I/O ports, memory and IRQ with > +the provided label. These are all unique per-device, except for IRQs which > +can be shared between devices. This leads to assignment problems since vmA_t > +can't access the IRQ devB_t. To work around this issue, flask-label-pci > +takes an optional 3rd argument to label the IRQ: > + > + flask-label-pci 0000:03:02.0 system_u:object_r:nic_dev_t \ > + system_u:object_r:shared_irq_t > + > +The IRQ labeling only applies to the PIRQ - MSI/MSI-X interrupts are labeled > +with the main device label. > + > +The policy needs to define the shared_irq_t with: > + type shared_irq_t, resource_type; > + > +And the policy needs to be updated to allow domains appropriate access. > > MLS/MCS policy > -------------- > diff --git a/tools/flask/utils/label-pci.c b/tools/flask/utils/label-pci.c > index 9ddb713cf4..897b772804 100644 > --- a/tools/flask/utils/label-pci.c > +++ b/tools/flask/utils/label-pci.c > @@ -28,7 +28,7 @@ > > static void usage (int argCnt, char *argv[]) > { > - fprintf(stderr, "Usage: %s SBDF label\n", argv[0]); > + fprintf(stderr, "Usage: %s SBDF label <irq_label>\n", argv[0]); > exit(1); > } > > @@ -39,12 +39,19 @@ int main (int argCnt, char *argv[]) > int seg, bus, dev, fn; > uint32_t sbdf; > uint64_t start, end, flags; > + char *pirq_label; > char buf[1024]; > FILE *f; > > - if (argCnt != 3) > + if (argCnt < 3 || argCnt > 4) style nit: space inside parens > usage(argCnt, argv); > > + if (argCnt == 4) { > + pirq_label = argv[3]; > + } else { > + pirq_label = argv[2]; > + } > + style nit: space inside parens and curly brackets could be dropped or should be moved to their own lines. > xch = xc_interface_open(0,0,0); > if ( !xch ) > { > @@ -107,7 +114,7 @@ int main (int argCnt, char *argv[]) > if (fscanf(f, "%" SCNu64, &start) != 1) > start = 0; > if (start) { > - ret = xc_flask_add_pirq(xch, start, argv[2]); > + ret = xc_flask_add_pirq(xch, start, pirq_label); > if (ret) { > fprintf(stderr, "xc_flask_add_pirq %"PRIu64" failed: %d\n", > start, ret); Style nits aside, LGTM. Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com>
On Mon, Mar 13, 2023 at 2:49 PM Daniel P. Smith <dpsmith@apertussolutions.com> wrote: > On 3/13/23 13:50, Jason Andryuk wrote: > > usage(argCnt, argv); > > > > + if (argCnt == 4) { > > + pirq_label = argv[3]; > > + } else { > > + pirq_label = argv[2]; > > + } > > + > > style nit: space inside parens and curly brackets could be dropped or > should be moved to their own lines. This file doesn't follow Xen style. I think dropping the curly braces is fine, but the lack of spaces 'if (argCnt == 4)' should stay for consistency. Does that sound okay? > > xch = xc_interface_open(0,0,0); > > if ( !xch ) > > { > > @@ -107,7 +114,7 @@ int main (int argCnt, char *argv[]) > > if (fscanf(f, "%" SCNu64, &start) != 1) > > start = 0; > > if (start) { > > - ret = xc_flask_add_pirq(xch, start, argv[2]); > > + ret = xc_flask_add_pirq(xch, start, pirq_label); > > if (ret) { > > fprintf(stderr, "xc_flask_add_pirq %"PRIu64" failed: %d\n", > > start, ret); > > Style nits aside, LGTM. > > Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com> Thanks, Jason
On 3/13/23 15:14, Jason Andryuk wrote: > On Mon, Mar 13, 2023 at 2:49 PM Daniel P. Smith > <dpsmith@apertussolutions.com> wrote: >> On 3/13/23 13:50, Jason Andryuk wrote: >>> usage(argCnt, argv); >>> >>> + if (argCnt == 4) { >>> + pirq_label = argv[3]; >>> + } else { >>> + pirq_label = argv[2]; >>> + } >>> + >> >> style nit: space inside parens and curly brackets could be dropped or >> should be moved to their own lines. > > This file doesn't follow Xen style. I think dropping the curly braces > is fine, but the lack of spaces 'if (argCnt == 4)' should stay for > consistency. Does that sound okay? > Hmm, I thought there was interest in getting everything in tree consistent, maybe I am mistaken. I am not hard pressed to enforce the style. Unless someone else objects, I am good with your proposal. >>> xch = xc_interface_open(0,0,0); >>> if ( !xch ) >>> { >>> @@ -107,7 +114,7 @@ int main (int argCnt, char *argv[]) >>> if (fscanf(f, "%" SCNu64, &start) != 1) >>> start = 0; >>> if (start) { >>> - ret = xc_flask_add_pirq(xch, start, argv[2]); >>> + ret = xc_flask_add_pirq(xch, start, pirq_label); >>> if (ret) { >>> fprintf(stderr, "xc_flask_add_pirq %"PRIu64" failed: %d\n", >>> start, ret); >> >> Style nits aside, LGTM. >> >> Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com> > > Thanks, > Jason >
On 13.03.2023 22:11, dpsmith.dev wrote: > On 3/13/23 15:14, Jason Andryuk wrote: >> On Mon, Mar 13, 2023 at 2:49 PM Daniel P. Smith >> <dpsmith@apertussolutions.com> wrote: >>> On 3/13/23 13:50, Jason Andryuk wrote: >>>> usage(argCnt, argv); >>>> >>>> + if (argCnt == 4) { >>>> + pirq_label = argv[3]; >>>> + } else { >>>> + pirq_label = argv[2]; >>>> + } >>>> + >>> >>> style nit: space inside parens and curly brackets could be dropped or >>> should be moved to their own lines. >> >> This file doesn't follow Xen style. I think dropping the curly braces >> is fine, but the lack of spaces 'if (argCnt == 4)' should stay for >> consistency. Does that sound okay? >> > > Hmm, I thought there was interest in getting everything in tree consistent, maybe I am mistaken. I am not hard pressed to enforce the style. Unless someone else objects, I am good with your proposal. The rule of thumb is that if a file is (largely) consistent in itself, then that style is preferred over introducing a mix. (I haven't checked this specific file, though.) The same may or may not apply to individual functions within a file; there it's more likely to be considered one way or the other on a case by case basis. Jan
On Tue, Mar 14, 2023 at 2:42 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 13.03.2023 22:11, dpsmith.dev wrote: > > On 3/13/23 15:14, Jason Andryuk wrote: > >> On Mon, Mar 13, 2023 at 2:49 PM Daniel P. Smith > >> <dpsmith@apertussolutions.com> wrote: > >>> On 3/13/23 13:50, Jason Andryuk wrote: > >>>> usage(argCnt, argv); > >>>> > >>>> + if (argCnt == 4) { > >>>> + pirq_label = argv[3]; > >>>> + } else { > >>>> + pirq_label = argv[2]; > >>>> + } > >>>> + > >>> > >>> style nit: space inside parens and curly brackets could be dropped or > >>> should be moved to their own lines. > >> > >> This file doesn't follow Xen style. I think dropping the curly braces > >> is fine, but the lack of spaces 'if (argCnt == 4)' should stay for > >> consistency. Does that sound okay? > >> > > > > Hmm, I thought there was interest in getting everything in tree consistent, maybe I am mistaken. I am not hard pressed to enforce the style. Unless someone else objects, I am good with your proposal. > > The rule of thumb is that if a file is (largely) consistent in itself, > then that style is preferred over introducing a mix. (I haven't checked > this specific file, though.) The same may or may not apply to individual > functions within a file; there it's more likely to be considered one way > or the other on a case by case basis. Thanks, Jan. The file has 2 functions. There are two instances of spaces within parens, and all the other cases, the majority, omit spaces. The next version will drop curly braces and continue omitting spaces. This patch should have had v2 in the subject (it has a v2 change log). The next one will have v3. Regards, Jason
On 14.03.2023 14:20, Jason Andryuk wrote: > On Tue, Mar 14, 2023 at 2:42 AM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 13.03.2023 22:11, dpsmith.dev wrote: >>> On 3/13/23 15:14, Jason Andryuk wrote: >>>> On Mon, Mar 13, 2023 at 2:49 PM Daniel P. Smith >>>> <dpsmith@apertussolutions.com> wrote: >>>>> On 3/13/23 13:50, Jason Andryuk wrote: >>>>>> usage(argCnt, argv); >>>>>> >>>>>> + if (argCnt == 4) { >>>>>> + pirq_label = argv[3]; >>>>>> + } else { >>>>>> + pirq_label = argv[2]; >>>>>> + } >>>>>> + >>>>> >>>>> style nit: space inside parens and curly brackets could be dropped or >>>>> should be moved to their own lines. >>>> >>>> This file doesn't follow Xen style. I think dropping the curly braces >>>> is fine, but the lack of spaces 'if (argCnt == 4)' should stay for >>>> consistency. Does that sound okay? >>>> >>> >>> Hmm, I thought there was interest in getting everything in tree consistent, maybe I am mistaken. I am not hard pressed to enforce the style. Unless someone else objects, I am good with your proposal. >> >> The rule of thumb is that if a file is (largely) consistent in itself, >> then that style is preferred over introducing a mix. (I haven't checked >> this specific file, though.) The same may or may not apply to individual >> functions within a file; there it's more likely to be considered one way >> or the other on a case by case basis. > > Thanks, Jan. The file has 2 functions. There are two instances of > spaces within parens, and all the other cases, the majority, omit > spaces. The next version will drop curly braces and continue omitting > spaces. > > This patch should have had v2 in the subject (it has a v2 change log). > The next one will have v3. Will it? I've committed it earlier today ... Jan
On Tue, Mar 14, 2023 at 10:11 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 14.03.2023 14:20, Jason Andryuk wrote: > > This patch should have had v2 in the subject (it has a v2 change log). > > The next one will have v3. > > Will it? I've committed it earlier today ... Not any more! Thank you. I had not checked staging. Regards, Jason
diff --git a/docs/misc/xsm-flask.txt b/docs/misc/xsm-flask.txt index 2419c5cf29..ba89ebbfd8 100644 --- a/docs/misc/xsm-flask.txt +++ b/docs/misc/xsm-flask.txt @@ -205,6 +205,22 @@ parameter, which can also be changed using xl setenforce). When using the default types for domains (domU_t), the example policy shipped with Xen should allow the same operations on or between domains as when not using FLASK. +By default, flask-label-pci labels the device, I/O ports, memory and IRQ with +the provided label. These are all unique per-device, except for IRQs which +can be shared between devices. This leads to assignment problems since vmA_t +can't access the IRQ devB_t. To work around this issue, flask-label-pci +takes an optional 3rd argument to label the IRQ: + + flask-label-pci 0000:03:02.0 system_u:object_r:nic_dev_t \ + system_u:object_r:shared_irq_t + +The IRQ labeling only applies to the PIRQ - MSI/MSI-X interrupts are labeled +with the main device label. + +The policy needs to define the shared_irq_t with: + type shared_irq_t, resource_type; + +And the policy needs to be updated to allow domains appropriate access. MLS/MCS policy -------------- diff --git a/tools/flask/utils/label-pci.c b/tools/flask/utils/label-pci.c index 9ddb713cf4..897b772804 100644 --- a/tools/flask/utils/label-pci.c +++ b/tools/flask/utils/label-pci.c @@ -28,7 +28,7 @@ static void usage (int argCnt, char *argv[]) { - fprintf(stderr, "Usage: %s SBDF label\n", argv[0]); + fprintf(stderr, "Usage: %s SBDF label <irq_label>\n", argv[0]); exit(1); } @@ -39,12 +39,19 @@ int main (int argCnt, char *argv[]) int seg, bus, dev, fn; uint32_t sbdf; uint64_t start, end, flags; + char *pirq_label; char buf[1024]; FILE *f; - if (argCnt != 3) + if (argCnt < 3 || argCnt > 4) usage(argCnt, argv); + if (argCnt == 4) { + pirq_label = argv[3]; + } else { + pirq_label = argv[2]; + } + xch = xc_interface_open(0,0,0); if ( !xch ) { @@ -107,7 +114,7 @@ int main (int argCnt, char *argv[]) if (fscanf(f, "%" SCNu64, &start) != 1) start = 0; if (start) { - ret = xc_flask_add_pirq(xch, start, argv[2]); + ret = xc_flask_add_pirq(xch, start, pirq_label); if (ret) { fprintf(stderr, "xc_flask_add_pirq %"PRIu64" failed: %d\n", start, ret);