2005-03-09 20:58:34

by Alan

[permalink] [raw]
Subject: Re: [PATCH] make st seekable again

On Maw, 2005-03-08 at 17:25, Linux Kernel Mailing List wrote:
> ChangeSet 1.2030, 2005/03/08 09:25:05-08:00, [email protected]
>
> [PATCH] make st seekable again
>
> Apparently `tar' errors out if it cannot perform lseek() against a tape. Work
> around that in-kernel.

Unfortunately this isn't a good idea. Allowing tar to read the tape
position makes sense, allowing it to zero the position might but you
have to do major surgery on the driver first because

1. It doesn't use ppos
2. It doesn't do locking on the ppos at all

Also allowing apps to randomly seek and report "ok" when they are
backing up to tape and might really need to see the error is not what
I'd call stable, professional or quality code.

I oppose this change for 2.6.11.3, I think 2.6.12 needs to address the
rest of the mess in that code to make it work (or implement a 'read
only' llseek and
use ppos right)

And -ac won't carry this change.


2005-03-09 21:59:07

by Kai Mäkisara (Kolumbus)

[permalink] [raw]
Subject: Re: [PATCH] make st seekable again

On Wed, 9 Mar 2005, Alan Cox wrote:

> On Maw, 2005-03-08 at 17:25, Linux Kernel Mailing List wrote:
> > ChangeSet 1.2030, 2005/03/08 09:25:05-08:00, [email protected]
> >
> > [PATCH] make st seekable again
> >
> > Apparently `tar' errors out if it cannot perform lseek() against a tape. Work
> > around that in-kernel.
>
> Unfortunately this isn't a good idea. Allowing tar to read the tape
> position makes sense, allowing it to zero the position might but you
> have to do major surgery on the driver first because
>
> 1. It doesn't use ppos
> 2. It doesn't do locking on the ppos at all
>
> Also allowing apps to randomly seek and report "ok" when they are
> backing up to tape and might really need to see the error is not what
> I'd call stable, professional or quality code.
>
The proper fix is to fix tar. I have sent an analysis of the problem and a
suggestion how to fix this to the bug-tar list on March 5 but it is still
waiting for moderator approval.

While waiting for the application to be fixed, it was decided to restore
the old behaviour of the tape drivers.

lseek on a tape is not a good fit (addressed by block, blocks on tape can
have any size, etc.). I don't know any Unix that would really implement
lseek on tapes but they usually don't return error. This is probably why
the tar bug has not been found earlier.

There has been one useful way of using lseek() with tapes in some systems.
Those refuse reads and writes if the file pointer reaches 2 GB. Resetting
it with lseek(fd,0,0) now and then has allowed writing/reading more than 2
GB.

I don't think implementing proper read-only lseek for tapes is worth the
trouble (reliable tracking of the current location is tricky). Purist
kernels can refuse lseeks. Pragmatic kernels can allow lseeks until
refusing those won't break common applications.

--
Kai

2005-03-09 23:05:15

by Alan

[permalink] [raw]
Subject: Re: [PATCH] make st seekable again

On Mer, 2005-03-09 at 21:58, Kai Makisara wrote:
> While waiting for the application to be fixed, it was decided to restore
> the old behaviour of the tape drivers.

Which means tar won't get fixed 8(

> I don't think implementing proper read-only lseek for tapes is worth the
> trouble (reliable tracking of the current location is tricky). Purist
> kernels can refuse lseeks. Pragmatic kernels can allow lseeks until
> refusing those won't break common applications.

The problem is the existing behaviour code isn't just 'not useful' its
badly broken. No locking, no overflow checks, updates the wrong variable
etc. It is asking for nasty accidents with critical user data.

Alan

2005-03-10 12:01:23

by Bill Davidsen

[permalink] [raw]
Subject: Re: [PATCH] make st seekable again

On Wed, 9 Mar 2005, Alan Cox wrote:

> On Mer, 2005-03-09 at 21:58, Kai Makisara wrote:
> > While waiting for the application to be fixed, it was decided to restore
> > the old behaviour of the tape drivers.
>
> Which means tar won't get fixed 8(

Bet that's true.
>
> > I don't think implementing proper read-only lseek for tapes is worth the
> > trouble (reliable tracking of the current location is tricky). Purist
> > kernels can refuse lseeks. Pragmatic kernels can allow lseeks until
> > refusing those won't break common applications.
>
> The problem is the existing behaviour code isn't just 'not useful' its
> badly broken. No locking, no overflow checks, updates the wrong variable
> etc. It is asking for nasty accidents with critical user data.

In other words, it should work correctly or not at all. At the least this
should be a config option, like UNSAFE_TAPE_POSITIONING or some such.
And show the option if the build includes BROKEN features. That should put
the decision where it belongs and clarify the possible failures.

Caould you live with that, Alan?

--
bill davidsen <[email protected]>
CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

2005-03-10 12:11:24

by Arjan van de Ven

[permalink] [raw]
Subject: Re: [PATCH] make st seekable again

> critical user data.
>
> In other words, it should work correctly or not at all. At the least this
> should be a config option, like UNSAFE_TAPE_POSITIONING or some such.
> And show the option if the build includes BROKEN features. That should put
> the decision where it belongs and clarify the possible failures.

CONFIG_SECURITY_HOLES doesn't make sense.
Better to just fix the security holes instead.


2005-03-10 16:46:26

by Alan

[permalink] [raw]
Subject: Re: [PATCH] make st seekable again

On Iau, 2005-03-10 at 12:10, Arjan van de Ven wrote:
> CONFIG_SECURITY_HOLES doesn't make sense.
> Better to just fix the security holes instead.

In the case of st its merely broken. I reviewed the code again to double
check for Marcelo. Still should be a "ask your vendor to fix tar" item.

Alan

2005-03-10 17:18:20

by Bill Davidsen

[permalink] [raw]
Subject: Re: [PATCH] make st seekable again

On Thu, 10 Mar 2005, Arjan van de Ven wrote:

> > critical user data.
> >
> > In other words, it should work correctly or not at all. At the least this
> > should be a config option, like UNSAFE_TAPE_POSITIONING or some such.
> > And show the option if the build includes BROKEN features. That should put
> > the decision where it belongs and clarify the possible failures.
>
> CONFIG_SECURITY_HOLES doesn't make sense.
> Better to just fix the security holes instead.

I think you're an idealist. If this were something other than tar it would
be simple, and you would be right. Well, you ARE right, but a change which
breaks tar, which many sites use for backups, is really not practical,
without a loophole until tar gets fixed. And as Alan noted, keeping track
of the physical position is very hard, and getting a tar fix might take a
while.

None of the choices is good; I see:
- leave it the way it is
- fix the hole and break tar
- wait for FSF to fix tar, then fix the hole
- try to fix it without breaking tar, which may not be really possible
and could leave part of the problem and still break tar somehow
- fix it, and leave the admin a way to build a kernel with the hole other
than just reverting the fix

I proposed the last, I won't cry if no one else likes it, it just seemed
realistic for people who don't use certain features of tar.

--
bill davidsen <[email protected]>
CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

2005-03-10 17:18:21

by Arjan van de Ven

[permalink] [raw]
Subject: Re: [PATCH] make st seekable again

On Thu, 2005-03-10 at 11:56 -0500, Bill Davidsen wrote:
> On Thu, 10 Mar 2005, Arjan van de Ven wrote:
>
> > > critical user data.
> > >
> > > In other words, it should work correctly or not at all. At the least this
> > > should be a config option, like UNSAFE_TAPE_POSITIONING or some such.
> > > And show the option if the build includes BROKEN features. That should put
> > > the decision where it belongs and clarify the possible failures.
> >
> > CONFIG_SECURITY_HOLES doesn't make sense.
> > Better to just fix the security holes instead.
>
> I think you're an idealist. If this were something other than tar it would
> be simple, and you would be right. Well, you ARE right, but a change which
> breaks tar, which many sites use for backups, is really not practical,
> without a loophole until tar gets fixed. And as Alan noted, keeping track
> of the physical position is very hard, and getting a tar fix might take a
> while.
>

No the problem is that the *st* code has a security hole. THAT is the
problem. Not that it would be seekable. But how it implements the seeks.
THAT part is the problem. And THAT can be fixed.

2005-03-10 18:15:46

by Alan

[permalink] [raw]
Subject: Re: [PATCH] make st seekable again

On Iau, 2005-03-10 at 16:56, Bill Davidsen wrote:
> - leave it the way it is
> - fix the hole and break tar
> - wait for FSF to fix tar, then fix the hole
> - try to fix it without breaking tar, which may not be really possible
> and could leave part of the problem and still break tar somehow
> - fix it, and leave the admin a way to build a kernel with the hole other
> than just reverting the fix

If we "fix" it the FSF will probably take years. If your vendor can't
produce a fixed tar when asked and the issue comes up, get a better
vendor ;)

Alan