Xen devel - Jul 2013 - RFC: removing hardcoded "modprobe blktap" in xencommons

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.

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

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

>>> 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

>>> 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

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

>>> 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

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

>>> 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

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

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

>>> 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

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.

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.

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

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.

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.

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.