RFC: removing hardcoded "modprobe blktap" in xencommons

RFC: removing hardcoded "modprobe blktap" in xencommons

[Wei Liu]

Hi Jan

Back before 4.3 release you complaint about the hardcoded modprobe for
blktap / blktap2 in xencommons script. I started to look into it this
week and tried to do this modprobe automatically in libxl (that's the
idea we discussed before 4.3 IIRC).

Unfortunately I didn't manage to find any canonical documents on how a
user space process can trigger a modprobe. I've looked at kernel
documentations and couldn't find any. Do you have any reference on how
to do that?

And, why do you think it is bad to have "modprobe blktap" in xencommons?
What about not removing it?


Wei.

Re: RFC: removing hardcoded "modprobe blktap" in xencommons

[Konrad Rzeszutek Wilk]

On Fri, Jul 12, 2013 at 05:12:55PM +0100, Wei Liu wrote:
> Hi Jan
> 
> Back before 4.3 release you complaint about the hardcoded modprobe for
> blktap / blktap2 in xencommons script. I started to look into it this
> week and tried to do this modprobe automatically in libxl (that's the
> idea we discussed before 4.3 IIRC).
> 
> Unfortunately I didn't manage to find any canonical documents on how a
> user space process can trigger a modprobe. I've looked at kernel
> documentations and couldn't find any. Do you have any reference on how
> to do that?

Ian and George talked to me on IRC about this. What we found (thanks for
Greg KH's help) was that you can use 'MODULE_ALIAS("devname:XYZ")'.

If any user application did an fopen("/dev/XYZ") then said module would be
automatically loaded.

> 
> And, why do you think it is bad to have "modprobe blktap" in xencommons?
> What about not removing it?
> 
> 
> Wei.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: RFC: removing hardcoded "modprobe blktap" in xencommons

[Wei Liu]

On Fri, Jul 12, 2013 at 02:00:20PM -0400, Konrad Rzeszutek Wilk wrote:
> On Fri, Jul 12, 2013 at 05:12:55PM +0100, Wei Liu wrote:
> > Hi Jan
> > 
> > Back before 4.3 release you complaint about the hardcoded modprobe for
> > blktap / blktap2 in xencommons script. I started to look into it this
> > week and tried to do this modprobe automatically in libxl (that's the
> > idea we discussed before 4.3 IIRC).
> > 
> > Unfortunately I didn't manage to find any canonical documents on how a
> > user space process can trigger a modprobe. I've looked at kernel
> > documentations and couldn't find any. Do you have any reference on how
> > to do that?
> 
> Ian and George talked to me on IRC about this. What we found (thanks for
> Greg KH's help) was that you can use 'MODULE_ALIAS("devname:XYZ")'.
> 

Yes I saw that.

> If any user application did an fopen("/dev/XYZ") then said module would be
> automatically loaded.
> 

I don't know much about blktap, but this doesn't seem to work in the
first place, otherwise we would not have to add that modprobe in
xencommons.

As for other modules I agree that they should be handled with the
autoloading scheme. However blktap is unmaintained I don't think how
feasible it is to "fix" blktap.

> > 
> > And, why do you think it is bad to have "modprobe blktap" in xencommons?
> > What about not removing it?
> > 
> > 
> > Wei.
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel

Re: RFC: removing hardcoded "modprobe blktap" in xencommons

[Jan Beulich]

>>> On 12.07.13 at 18:12, Wei Liu <wei.liu2@citrix.com> wrote:
> Back before 4.3 release you complaint about the hardcoded modprobe for
> blktap / blktap2 in xencommons script. I started to look into it this
> week and tried to do this modprobe automatically in libxl (that's the
> idea we discussed before 4.3 IIRC).
> 
> Unfortunately I didn't manage to find any canonical documents on how a
> user space process can trigger a modprobe. I've looked at kernel
> documentations and couldn't find any. Do you have any reference on how
> to do that?

Not sure what you're asking about. system("modprobe <modname>")
is what I'd start with, or utilizing any process spawning internal
interface libxl already has.

> And, why do you think it is bad to have "modprobe blktap" in xencommons?
> What about not removing it?

I think the discussion why this is a bad idea was long settled: It is
bad practice to load all sorts of stuff regardless of whether it's
actually used - not only from a resource usage pov, but also from
a security one (less kernel code running implies less kernel bugs to
worry about allowing an eventual exploit).

Jan

Re: RFC: removing hardcoded "modprobe blktap" in xencommons

[Jan Beulich]

>>> On 12.07.13 at 20:00, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Fri, Jul 12, 2013 at 05:12:55PM +0100, Wei Liu wrote:
>> Back before 4.3 release you complaint about the hardcoded modprobe for
>> blktap / blktap2 in xencommons script. I started to look into it this
>> week and tried to do this modprobe automatically in libxl (that's the
>> idea we discussed before 4.3 IIRC).

But you understand that the complaint isn't just about blktap, but
about _all_ of the modprobes there.

>> Unfortunately I didn't manage to find any canonical documents on how a
>> user space process can trigger a modprobe. I've looked at kernel
>> documentations and couldn't find any. Do you have any reference on how
>> to do that?
> 
> Ian and George talked to me on IRC about this. What we found (thanks for
> Greg KH's help) was that you can use 'MODULE_ALIAS("devname:XYZ")'.
> 
> If any user application did an fopen("/dev/XYZ") then said module would be
> automatically loaded.

This works only for drivers for devices with known major:minor, as
otherwise you have a chicken-and-egg problem between udev
needing a way to determine the major:minor pair to create the
/dev node, and the driver wanting to be loaded via udev rules.

Which particularly means that - afaict - drivers (like blktap) using
MISC_DYNAMIC_MINOR aren't suitable for this; I'll be happily
corrected if there nevertheless is a way...

Yet I don't see why this would be needed for backends anyway:
The tool stack knows which backend it needs, and hence can
modprobe its xen-backend:* alias easily enough (i.e. still without
knowing the actual module name).

Jan

Re: RFC: removing hardcoded "modprobe blktap" in xencommons

[Wei Liu]

On Mon, Jul 15, 2013 at 07:06:52AM +0100, Jan Beulich wrote:
> >>> On 12.07.13 at 18:12, Wei Liu <wei.liu2@citrix.com> wrote:
> > Back before 4.3 release you complaint about the hardcoded modprobe for
> > blktap / blktap2 in xencommons script. I started to look into it this
> > week and tried to do this modprobe automatically in libxl (that's the
> > idea we discussed before 4.3 IIRC).
> > 
> > Unfortunately I didn't manage to find any canonical documents on how a
> > user space process can trigger a modprobe. I've looked at kernel
> > documentations and couldn't find any. Do you have any reference on how
> > to do that?
> 
> Not sure what you're asking about. system("modprobe <modname>")
> is what I'd start with, or utilizing any process spawning internal
> interface libxl already has.
> 

I'm asking because:

1) removing that specific one-liner requires giantic amount of work --
   around 200 lines of code plumbing into libxl's AO interface
2) not all libxl consumers has the necessary privilege to run modprobe
   AIUI.

> > And, why do you think it is bad to have "modprobe blktap" in xencommons?
> > What about not removing it?
> 
> I think the discussion why this is a bad idea was long settled: It is
> bad practice to load all sorts of stuff regardless of whether it's
> actually used - not only from a resource usage pov, but also from
> a security one (less kernel code running implies less kernel bugs to
> worry about allowing an eventual exploit).
> 

Agreed. This issue should be ideally fixed for all drivers using the
loading methanism mentioned by Konrad. However blktap is unmaintained I
don't know how feasible it is to modify its source code and push this
change to ancient kernels.


Wei.

> Jan

Re: RFC: removing hardcoded "modprobe blktap" in xencommons

[Jan Beulich]

>>> On 15.07.13 at 09:33, Wei Liu <wei.liu2@citrix.com> wrote:
> On Mon, Jul 15, 2013 at 07:06:52AM +0100, Jan Beulich wrote:
>> >>> On 12.07.13 at 18:12, Wei Liu <wei.liu2@citrix.com> wrote:
>> > Back before 4.3 release you complaint about the hardcoded modprobe for
>> > blktap / blktap2 in xencommons script. I started to look into it this
>> > week and tried to do this modprobe automatically in libxl (that's the
>> > idea we discussed before 4.3 IIRC).
>> > 
>> > Unfortunately I didn't manage to find any canonical documents on how a
>> > user space process can trigger a modprobe. I've looked at kernel
>> > documentations and couldn't find any. Do you have any reference on how
>> > to do that?
>> 
>> Not sure what you're asking about. system("modprobe <modname>")
>> is what I'd start with, or utilizing any process spawning internal
>> interface libxl already has.
>> 
> 
> I'm asking because:
> 
> 1) removing that specific one-liner requires giantic amount of work --
>    around 200 lines of code plumbing into libxl's AO interface

There's going to be changes, yes, but as said these should benefit
all backends requiring kernel modules, not just blktap. Hence the
payoff will be bigger.

> 2) not all libxl consumers has the necessary privilege to run modprobe
>    AIUI.

A tool stack controlling kernel based backends necessarily has to
have a way to load kernel modules.

>> > And, why do you think it is bad to have "modprobe blktap" in xencommons?
>> > What about not removing it?
>> 
>> I think the discussion why this is a bad idea was long settled: It is
>> bad practice to load all sorts of stuff regardless of whether it's
>> actually used - not only from a resource usage pov, but also from
>> a security one (less kernel code running implies less kernel bugs to
>> worry about allowing an eventual exploit).
>> 
> 
> Agreed. This issue should be ideally fixed for all drivers using the
> loading methanism mentioned by Konrad. However blktap is unmaintained I
> don't know how feasible it is to modify its source code and push this
> change to ancient kernels.

I don't see what modifications you want/need to make.

Jan

Re: RFC: removing hardcoded "modprobe blktap" in xencommons

[Wei Liu]

On Mon, Jul 15, 2013 at 08:44:53AM +0100, Jan Beulich wrote:
> >>> On 15.07.13 at 09:33, Wei Liu <wei.liu2@citrix.com> wrote:
> > On Mon, Jul 15, 2013 at 07:06:52AM +0100, Jan Beulich wrote:
> >> >>> On 12.07.13 at 18:12, Wei Liu <wei.liu2@citrix.com> wrote:
> >> > Back before 4.3 release you complaint about the hardcoded modprobe for
> >> > blktap / blktap2 in xencommons script. I started to look into it this
> >> > week and tried to do this modprobe automatically in libxl (that's the
> >> > idea we discussed before 4.3 IIRC).
> >> > 
> >> > Unfortunately I didn't manage to find any canonical documents on how a
> >> > user space process can trigger a modprobe. I've looked at kernel
> >> > documentations and couldn't find any. Do you have any reference on how
> >> > to do that?
> >> 
> >> Not sure what you're asking about. system("modprobe <modname>")
> >> is what I'd start with, or utilizing any process spawning internal
> >> interface libxl already has.
> >> 
> > 
> > I'm asking because:
> > 
> > 1) removing that specific one-liner requires giantic amount of work --
> >    around 200 lines of code plumbing into libxl's AO interface
> 
> There's going to be changes, yes, but as said these should benefit
> all backends requiring kernel modules, not just blktap. Hence the
> payoff will be bigger.
> 
> > 2) not all libxl consumers has the necessary privilege to run modprobe
> >    AIUI.
> 
> A tool stack controlling kernel based backends necessarily has to
> have a way to load kernel modules.
> 

Yes, I agree.

> >> > And, why do you think it is bad to have "modprobe blktap" in xencommons?
> >> > What about not removing it?
> >> 
> >> I think the discussion why this is a bad idea was long settled: It is
> >> bad practice to load all sorts of stuff regardless of whether it's
> >> actually used - not only from a resource usage pov, but also from
> >> a security one (less kernel code running implies less kernel bugs to
> >> worry about allowing an eventual exploit).
> >> 
> > 
> > Agreed. This issue should be ideally fixed for all drivers using the
> > loading methanism mentioned by Konrad. However blktap is unmaintained I
> > don't know how feasible it is to modify its source code and push this
> > change to ancient kernels.
> 
> I don't see what modifications you want/need to make.
> 

So my point is, if every driver follows the kernel convention
(open(/dev/XYZ) would cause the module to load automatically) there then
would be minimal changes to libxl. It is unmaintained kernel modules
like blktap makes it not feasible to make things simplier.

(Wild guess below, correct me if I'm wrong)

Say, if we could make it just like open(blkback_char_dev), the loading
is just a one-liner and doesn't need to be using libxl's AO interface.


Wei.

> Jan

Re: RFC: removing hardcoded "modprobe blktap" in xencommons

[Jan Beulich]

>>> On 15.07.13 at 10:26, Wei Liu <wei.liu2@citrix.com> wrote:
> So my point is, if every driver follows the kernel convention
> (open(/dev/XYZ) would cause the module to load automatically) there then
> would be minimal changes to libxl. It is unmaintained kernel modules
> like blktap makes it not feasible to make things simplier.
> 
> (Wild guess below, correct me if I'm wrong)
> 
> Say, if we could make it just like open(blkback_char_dev), the loading
> is just a one-liner and doesn't need to be using libxl's AO interface.

Once again - most if not all backends don't fit that model - blktap
as said doesn't and neither blkback nor netback don't as they don't
create any /dev nodes.

Backend module loading might be triggerable through xenstore
node watches...

Jan

Re: RFC: removing hardcoded "modprobe blktap" in xencommons

[Wei Liu]

On Mon, Jul 15, 2013 at 10:34:02AM +0100, Jan Beulich wrote:
> >>> On 15.07.13 at 10:26, Wei Liu <wei.liu2@citrix.com> wrote:
> > So my point is, if every driver follows the kernel convention
> > (open(/dev/XYZ) would cause the module to load automatically) there then
> > would be minimal changes to libxl. It is unmaintained kernel modules
> > like blktap makes it not feasible to make things simplier.
> > 
> > (Wild guess below, correct me if I'm wrong)
> > 
> > Say, if we could make it just like open(blkback_char_dev), the loading
> > is just a one-liner and doesn't need to be using libxl's AO interface.
> 
> Once again - most if not all backends don't fit that model - blktap
> as said doesn't and neither blkback nor netback don't as they don't
> create any /dev nodes.
> 

OK, I get what you mean.

> Backend module loading might be triggerable through xenstore
> node watches...
> 

Sure.


Wei.

> Jan

Re: RFC: removing hardcoded "modprobe blktap" in xencommons

[George Dunlap]

On 15/07/13 10:38, Wei Liu wrote:
> On Mon, Jul 15, 2013 at 10:34:02AM +0100, Jan Beulich wrote:
>>>>> On 15.07.13 at 10:26, Wei Liu <wei.liu2@citrix.com> wrote:
>>> So my point is, if every driver follows the kernel convention
>>> (open(/dev/XYZ) would cause the module to load automatically) there then
>>> would be minimal changes to libxl. It is unmaintained kernel modules
>>> like blktap makes it not feasible to make things simplier.
>>>
>>> (Wild guess below, correct me if I'm wrong)
>>>
>>> Say, if we could make it just like open(blkback_char_dev), the loading
>>> is just a one-liner and doesn't need to be using libxl's AO interface.
>> Once again - most if not all backends don't fit that model - blktap
>> as said doesn't and neither blkback nor netback don't as they don't
>> create any /dev nodes.
>>
> OK, I get what you mean.
>
>> Backend module loading might be triggerable through xenstore
>> node watches...
>>
> Sure.

If we did something like this, we'd still have to have the modprobes in 
xencommons for older kernels; we'd just have to have a way to disable it 
for newer kernels.

  -George

Re: RFC: removing hardcoded "modprobe blktap" in xencommons

[Jan Beulich]

>>> On 15.07.13 at 16:08, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 15/07/13 10:38, Wei Liu wrote:
>> On Mon, Jul 15, 2013 at 10:34:02AM +0100, Jan Beulich wrote:
>>>>>> On 15.07.13 at 10:26, Wei Liu <wei.liu2@citrix.com> wrote:
>>>> So my point is, if every driver follows the kernel convention
>>>> (open(/dev/XYZ) would cause the module to load automatically) there then
>>>> would be minimal changes to libxl. It is unmaintained kernel modules
>>>> like blktap makes it not feasible to make things simplier.
>>>>
>>>> (Wild guess below, correct me if I'm wrong)
>>>>
>>>> Say, if we could make it just like open(blkback_char_dev), the loading
>>>> is just a one-liner and doesn't need to be using libxl's AO interface.
>>> Once again - most if not all backends don't fit that model - blktap
>>> as said doesn't and neither blkback nor netback don't as they don't
>>> create any /dev nodes.
>>>
>> OK, I get what you mean.
>>
>>> Backend module loading might be triggerable through xenstore
>>> node watches...
>>>
>> Sure.
> 
> If we did something like this, we'd still have to have the modprobes in 
> xencommons for older kernels; we'd just have to have a way to disable it 
> for newer kernels.

Oh, I was actually thinking of user space (tool stack) watches. But
then again I'm not really sure this idea would work out at all...

Jan

Re: RFC: removing hardcoded "modprobe blktap" in xencommons

[Ian Jackson]

George Dunlap writes ("Re: RFC: removing hardcoded "modprobe blktap" in xencommons"):
> If we did something like this, we'd still have to have the modprobes in 
> xencommons for older kernels; we'd just have to have a way to disable it 
> for newer kernels.

Many of the modules we'd be asking for (eg blktap*) don't exist on
newer kernels at all, so the modprobe is harmless.

For the others we can check uname.

Ian.

Re: RFC: removing hardcoded "modprobe blktap" in xencommons

[Wei Liu]

On Wed, Jul 17, 2013 at 12:16:01PM +0100, Ian Jackson wrote:
> George Dunlap writes ("Re: RFC: removing hardcoded "modprobe blktap" in xencommons"):
> > If we did something like this, we'd still have to have the modprobes in 
> > xencommons for older kernels; we'd just have to have a way to disable it 
> > for newer kernels.
> 
> Many of the modules we'd be asking for (eg blktap*) don't exist on
> newer kernels at all, so the modprobe is harmless.
> 
> For the others we can check uname.
> 

I think Jan's main point is "defer loading modules till the last
minute". Checking for uname in xencommons cannot achieve that.


Wei.

> Ian.

Re: RFC: removing hardcoded "modprobe blktap" in xencommons

[George Dunlap]

On 07/18/2013 08:31 AM, Wei Liu wrote:
> On Wed, Jul 17, 2013 at 12:16:01PM +0100, Ian Jackson wrote:
>> George Dunlap writes ("Re: RFC: removing hardcoded "modprobe blktap" in xencommons"):
>>> If we did something like this, we'd still have to have the modprobes in
>>> xencommons for older kernels; we'd just have to have a way to disable it
>>> for newer kernels.
>>
>> Many of the modules we'd be asking for (eg blktap*) don't exist on
>> newer kernels at all, so the modprobe is harmless.
>>
>> For the others we can check uname.
>>
>
> I think Jan's main point is "defer loading modules till the last
> minute". Checking for uname in xencommons cannot achieve that.

It can defer modules *for systems that can load them automatically*, 
while loading them immediately for those that don't.  I think that's an 
improvement.

  -George

Re: RFC: removing hardcoded "modprobe blktap" in xencommons

[Ian Jackson]

George Dunlap writes ("Re: RFC: removing hardcoded "modprobe blktap" in xencommons"):
> It can defer modules *for systems that can load them automatically*, 
> while loading them immediately for those that don't.  I think that's an 
> improvement.

I think we are getting into trouble here because we are lumping
together all of these modprobes.

Wei is talking specifically about "modprobe blktap" (or indeed
"modprobe blktap2").  Modern kernels don't have this module at all, so
there is little harm in trying to load it.

Jan is talking about various other modules.  I agree with Jan that we
should, for example, make sure that ordinary backend drivers (of which
blktap is not one) are loaded using the automatic machinery rather
than explicitly in xencommons.

If there are old kernels whose automatic machinery is broken then
I think testing uname is probably an OK way to support them.  That
avoids having to try to get a bunch of backported module loading fixes
into numerous distros' stable kernels, etc.

Ian.

Re: RFC: removing hardcoded "modprobe blktap" in xencommons

[Wei Liu]

On Thu, Jul 18, 2013 at 12:12:48PM +0100, Ian Jackson wrote:
> George Dunlap writes ("Re: RFC: removing hardcoded "modprobe blktap" in xencommons"):
> > It can defer modules *for systems that can load them automatically*, 
> > while loading them immediately for those that don't.  I think that's an 
> > improvement.
> 
> I think we are getting into trouble here because we are lumping
> together all of these modprobes.
> 
> Wei is talking specifically about "modprobe blktap" (or indeed
> "modprobe blktap2").  Modern kernels don't have this module at all, so
> there is little harm in trying to load it.
> 

Indeed.

> Jan is talking about various other modules.  I agree with Jan that we
> should, for example, make sure that ordinary backend drivers (of which
> blktap is not one) are loaded using the automatic machinery rather
> than explicitly in xencommons.
> 

Yes, we seemed to mix these two things up.

> If there are old kernels whose automatic machinery is broken then
> I think testing uname is probably an OK way to support them.  That
> avoids having to try to get a bunch of backported module loading fixes
> into numerous distros' stable kernels, etc.
> 

Agreed. However for the blktap case, as you stated above there is
actually no harm doing that modprobe unconditionally.

So by far the proper fix would be:
a) fix all maintained modules to use kernel machinary
b) make use of kernel auto-load machinary in libxl
c) add uname test around module loading in xencommons

Is this correct?


Wei.

> Ian.

Re: RFC: removing hardcoded "modprobe blktap" in xencommons

[Ian Jackson]

Wei Liu writes ("Re: RFC: removing hardcoded "modprobe blktap" in xencommons"):
> So by far the proper fix would be:
> a) fix all maintained modules to use kernel machinary

Yes.

> b) make use of kernel auto-load machinary in libxl

Hopefully this will be a no-op.

> c) add uname test around module loading in xencommons

Yes.

Ian.