Bug 2596

Patch welcomed

I don't have a quick solution to this. At my new job I have much less time for this sort of thing and what time I do have is being applied to mythtv and avidemux.

If you have time to investigate the problem further and can point out where the fault is specifically, or better yet, provide a patch, it would be very welcomed.

RPMFusion is no longer releasing updates for this version of Fedora. This bug
will be set to RESOLVED:EXPIRED next week to reflect this.

If the problem persists after upgrading to the latest version of Fedora, please
update the version field of this bug (and re-open it if it has been closed).

This is a valid issue.
still your patch is awaited.
http://cvs.rpmfusion.org/viewvc/rpms/akmods/devel/?root=free

A side note is that you could have used mktemp -d instead and trap exit to delete it. (most foo-snapshot.sh script to create source tarball from upstream cvs use that).

No answear back from this particular issue.

Hi,

what does it mean: "No answer back from this particular issue."?

Was there a question?

I'm not sure about your bug policy, but it seems to me this is still a valid issue, so I'll reopen it, at least not to forget about it.

If there is any other reasons to close it, please proceed accordingly.
Of course, it would be better to fix it... :-)

Thanks,

bye,

pg

so again, patch welcomed.

This is the akmod code
http://cvs.rpmfusion.org/viewvc/rpms/akmods/devel/?root=free
Please send a unified diff of the tested changes to support TMDIR

Hi all,

I think I understood the problem, but I've no solution, maybe a workaround.

The issue is the $TMPDIR is exported for "root" user, but "akmods" runs a "akmods" user which, of course, does not have permissions for /tmp/root/.

Now, one possible option would be to re-export, inside /sbin/akmods, $TMPDIR as /tmp or run a /bin/mkdir -p /tmp/akmods/ -m 700 and export TMPDIR=/tmp/akmods...

Is this making sense to you?

I can try a small patch, if you agree.

Thanks,

bye,

pg

If that's all that's required, I could add an environment variable to the systemd service file...

Well, not sure about the variable, but this is a possible patch (which seems to work, at least for me):

--- akmods.org	2015-03-26 22:35:26.505186127 +0100
+++ akmods	2015-03-26 22:35:54.518017133 +0100
@@ -109,6 +109,8 @@
 
 create_tmpdir()
 {
+	runuser -u akmods -- mkdir -p /tmp/akmods -m 700
+	export TMPDIR=/tmp/akmods
 	if ! tmpdir="$(mktemp -d -t ${myprog}.XXXXXXXX)/" ; then 
 		akmods_echo 2 1  "ERROR: failed to create tmpdir."
 		akmods_echo 2 1 --failure; return 1

Hope this helps,

bye,

pg

Maybe one clarification...

/sbin/akmods runs as root.

This, in turns, calls /bin/akmodsbuild as user "akmods", this second one will not have permission for /tmp/root.

bye,

pg

I have not had time to test it yet, but in reading through the man page for mktemp, the -t option is depreciated. What if we just changed -t to "-p /tmp" then it should ignore the value of $TMPDIR.

Hi,

well, I guess changing the behaviour of "mktemp" could work as well.

In any case, since "-t" is deprecated, I would suggest the change anyway.

Furthermore, I made a quick test, this works, but you'll have to change the same in "/usr/bin/akmodbuild". This is still using "mktemp -d -t ..." (and failing due to $TMPDIR).

Hope this helps,

bye,

pg

Just checking for clarification...

You reverted your change (patch) and tried changing from -t to -p /tmp and everything worked as expected?

Yep,

I commented out the two lines I added in "/sbin/akmods" and changed the "mktemp" as you suggested.
This alone was not working, same error about directory access.
Then I changed the similar "mktemp" present in "/usr/bin/akmodsbuild" and this made the trick.

So, I tested with *both* "mktemp" changed (which, as I wrote before, it is maybe a safer choice).

I guess you do not need a patch for those changes... :-)

bye,

pg

I was able to test the changes as well and it seemed to work so I'm going to do official builds.

Builds completed for devel/f21, builds for f20 will be done when my cvs access is fixed.

Hi Richard,

I'm very sorry, but it seems there is still some problem, even if maybe it does not belong to "akmods".
In this case, please change the component properly.

I've 3 modules: VirtualBox, xtables-addons and nvidia-340xx.
The first two worked flawlessly, as tested before.

The third one seems to have the same issue, but, as I wrote above, maybe it is the module itself.

In the error log I can see:

...
Patch #0 (nv-linux-arm.patch):
+ /usr/bin/patch -p1 --fuzz=0
+ /usr/bin/cat /tmp/akmodsbuild.ll1Xp4Bc/SOURCES/nv-linux-arm.patch
/usr/bin/patch: **** Can't create temporary file /tmp/root/pp2S4ig0 : Permission denied
error: Bad exit status from /var/tmp/rpm-tmp.4MOEGh (%prep)
...

So, it seems the $TMPDIR is still propagated somewhere were it creates troubles.

Do you know by chance who is calling "patch"?

Thanks and sorry again,

bye,

pg

(In reply to comment #19)
..
> 
> Do you know by chance who is calling "patch"?
This is a patch needed by the nvidia-340xx driver package.
At this step, this is a generic patch directive, so the issue can be hit by any kmod.

Also I saw that you have created a temporary directory, but have you considered to clean it ? . Please consider to "trap the exit signal" while doing so (see ffmpeg-snapshot.sh for a sample).

I wouldn't be surprised that the rpm process would set it's own temporary directory. Usually this is located into /var/tmp instead of /tmp.
Also using /var/tmp can be considered if /tmp (that can be on tmpfs, hence consume RAM) isn't big enought.

(In reply to comment #20)
> (In reply to comment #19)
> ..
> > 
> > Do you know by chance who is calling "patch"?
> This is a patch needed by the nvidia-340xx driver package.
> At this step, this is a generic patch directive, so the issue can be hit by any
> kmod.

Is this in "akmods" or in "akmods-nvidia"?

> Also I saw that you have created a temporary directory, but have you considered
> to clean it ? . Please consider to "trap the exit signal" while doing so (see
> ffmpeg-snapshot.sh for a sample).

I'm not sure what you're referring to about the "temporary directory".

In my system, I've a script, in /etc/profile.d/, which creates a directory in /tmp for each user logging in and sets $TMPDIR to that directory. All with permissions 700.

This I use to avoid mixing up files in /tmp belonging to different users and keep everything clean and separate.

This directory does not need to be cleaned because 1) /tmp is tmpfs and 2) /tmp is /tmp... :-)

On the other hand, "akmods" creates, correctly, some folders in /tmp for its own usage, but I'm not sure about them.
I guess the creator/maintainer of "akmods" knows better.

The whole problem arises because "akmods" is called by root, but it switches to "akmods" user, which inherits the $TMPDIR from root, which is not accessible.
In this situation, "akmods" is not logged in, so he has no $TMPDIR.

Maybe the proper solution would be to explicitly set (or unset) $TMPDIR in the "akmods" script, like what the first patch was doing.
Of course, assuming that "root" will not write something in /tmp in the rest of the script...

bye,

pg

Hi all,

actually, I was thinking maybe the real issue is with "runuser".

I mean, from "root" user to any other user it is always possible that environmental variables are passed.
In general, this can be a problem, as we exactly see in this bug report.

So, either the usage of "runuser" in "akmods" is not complaint, or "runuser" has a bug.

Any suggestions or ideas on how to proceed?
Should we contact "runuser people to clarify the issue? Open a bug?
Or this is still an "akmods" problem?

Thanks,

bye,

pg

(In reply to comment #23)
> Hi all,
> 
> actually, I was thinking maybe the real issue is with "runuser".
Can you remind me if the runuser TMPDIR or whatever does work for anyone but you ?

Because I'm a little bit astonished by the description of the issue based on that creating such tmpdir from one user, then running building from another will not pass the way you are creating the tmpdir !

This is your own issue, don't spread the dirt

(In reply to comment #24)
> (In reply to comment #23)
> > Hi all,
> > 
> > actually, I was thinking maybe the real issue is with "runuser".
> Can you remind me if the runuser TMPDIR or whatever does work for anyone but
> you ?

It does not work for *everybody* which is setting $TMPDIR.
And setting $TMDIR is not some ultra-special-fantastic case, it is some normal operation allowed by Unix standard, even encouraged.

> Because I'm a little bit astonished by the description of the issue based on
> that creating such tmpdir from one user, then running building from another
> will not pass the way you are creating the tmpdir !

You seem not to understand how the variable export works in Unix, then.

You can, in any case, try by yourself.
It is not difficult.
As root:

mkdir -m 700 /tmp/test
export TMPDIR /tmp/test
akmods --build --force nvidia-...

You'll see this failing (before the fix, any "akmods" build would have failed, now, for me, only the nvidia, due to the patch story).

If root sets $TMPDIR to something (with 700 permissions) and then "runuser" runs a SW (different user), using /tmp, then, correctly, this software will user $TMPDIR, which is not available to that user.
And this will fail.

This means either "akmods" abuses "runuser" or "runuser" needs to take into account the environment in a different way (not export it, but re-create it).

> This is your own issue, don't spread the dirt

It is *not* my own issue, it is a general architectural issue, which seems nobody discovered until now.
Likely, because standard desktop/laptop users do not care to set $TMPDIR, but what if a distribution will do it?
Fedora, for example, has /run/user/<id>/ as /tmp for some process tmp files, they may realistically consider using $TMPDIR for other purposes.

Of course, one possibility is to tell the user they cannot set $TMPDIR, which seems to me a bit extreme...

Finally, all this story is in order to *improve* rpmfusion software, to make it better, to fortify it, not to create silly problems.

bye,

pg

You assume the same user run the whole script entirely.
Please compare the default perms for /tmp or /var/tmp and yours.
This cannot work !!! Other script will fails the same.

Now if you still wonder why this doesn't work after 3 years, I really cannot do much for you.

And for the sake of the user tmpdir feature here a sane way to setup per-user tmpdir:
https://skvidal.wordpress.com/2007/07/13/poly-instantiated-tmpdirs/

Now I really expects users request as such to be "provided by patch" (and tested).
The bug remains closed as INVALID until a working patch is attached here for review.

(In reply to comment #26)
> You assume the same user run the whole script entirely.

What do you mean?
I know the script does not run from the same user (root).
And I know that, due to this, there is a problem.

Or you meant something else?

> Please compare the default perms for /tmp or /var/tmp and yours.
> This cannot work !!! Other script will fails the same.

Which scripts? Why they will fail? What are you writing about?

No scripts will fail (and, actually, only "akmods" fails), because they are
started from the same user which has the permissions over $TMPDIR.
Only if there is an handover from one user to an other there will be problems.

And, BTW, I'm using this $TMPDIR approach on other PCs (with Ubuntu) since many
years and I *never* had problems.

> Now if you still wonder why this doesn't work after 3 years, I really cannot do
> much for you.

Did you tested as I suggested? Does it work for you?

I've the impression you are still missing something.

I'll try again.

Each user has its own temporary directory, defined by $TMPDIR, inside /tmp.
Each user can dispose of this dir as it please, since it is its own,
permissions are set for the user, so all scripts work.

As soon as root runs something via "runuser", $TMPDIR is *exported* to the
script and used as temporary directory. Since the user running the script does
not have, at this point, permission over the $TMPDIR (belonging to root), the
script fails.

Is it clear?

The problem is not is having a separate temporary directory, this works.
The problem is the handover from root to <user>, which is done *regardless* of
the environment set by root.

This is a bug because setting $TMPDIR is a recognized approach, is not
something specific to my setup.

bye,

pg

(In reply to comment #27)
> And for the sake of the user tmpdir feature here a sane way to setup per-user
> tmpdir:
> https://skvidal.wordpress.com/2007/07/13/poly-instantiated-tmpdirs/

This is a different thing, namely, a different target.
The $TMPDIR is a *valid* approach, for the very basic simple reason the shell supports it.

> Now I really expects users request as such to be "provided by patch" (and
> tested).

First of all, you're expecting too much.
Not all users have capabilities or time to provide patches.
You should be happy to have a bug report, this means people care.

Second, I provided a patch, which proved working only partially.

Third, I *asked* about the "patch" issue I *reported* here, but instead of getting clarification, I only got complains.

So, please provide an explanation on where to look for that.

As alternative, I provided an other patch which was clearing $TMPDIR, but then we decided to change the way "mktmpdir" was called, so it was skipped.

> The bug remains closed as INVALID until a working patch is attached here for
> review.

Patches are there, please apply.

bye,

pg

Created attachment 1435 [details]
Unset $TMPDIR before calling "runuser"

Hi again,

please find attached a patch against the latest "akmods" (since the previous patch was against the previous "akmods"), unsetting $TMPDIR before calling "runuser".

Please let me know if there are major issues.

Note that, in the comment, it is stated that "runuser" mis-uses $TMPDIR.

I checked on Ubuntu. They have "runas", which wipes out the environment completely and re-creates some variables (like $HOME, $USER or $SHELL).

"runuser", on the other hand, re-writes some variables (like the above), but it seems it does preserve all the rest, including $TMPDIR, even if "-m" is not given.

Now, IMHO the behavior of "runas" is more correct. In case you agree, it would be possible to report the issue upstream (Fedora).

Final note, I checked the patched "akmods" with the NVidia kernel module and it seems to work as expected.
Of course, surprises can also pop up...

Hope this help,

bye,

pg

I believe this was addressed in the in 0.5.2-1 release, closing.

Hi Richard,

as mentioned in the discussion, the patch I provided for 0.5.2-1 did not prove robust enough.

My suggestion would be to use the first (or last) patch (in addition), in order to unset $TMPDIR completely.

Here, again, a cleaner version:

--- a/akmods	2015-07-15 07:56:43.966231840 +0200
+++ b/akmods	2015-07-15 07:56:47.927801767 +0200
@@ -245,6 +245,8 @@
 	kmodlogfile="/var/cache/akmods/${this_kmodname}/.last.log"
 
 	# build module using akmod
+	# Unset TMPDIR since it is misused by "runuser"
+	unset TMPDIR
 	akmods_echo 1 4 "Building RPM using the command '$(which akmodsbuild) --target $(uname -m) --kernels ${this_kernelver} ${this_kmodsrpm}'"
 	/sbin/runuser -s /bin/bash -c "$(which akmodsbuild) --quiet --quiet --target $(uname -m) --kernels ${this_kernelver} --outputdir ${tmpdir}results --logfile ${tmpdir}/akmodsbuild.log ${this_kmodsrpm}" akmods >> "${kmodlogfile}" 2>&1
 	local returncode=$?

Hope this helps,

bye,

pg

Ok, I've added that to the next release.