Re: [PATCH] make st seekable again

Re: [PATCH] make st seekable again

[Alan Cox]

On Maw, 2005-03-08 at 17:25, Linux Kernel Mailing List wrote:
> ChangeSet 1.2030, 2005/03/08 09:25:05-08:00, kai.makisara@kolumbus.fi
> 
> 	[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.

Re: [PATCH] make st seekable again

[Kai Makisara]

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, kai.makisara@kolumbus.fi
> > 
> > 	[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

Re: [PATCH] make st seekable again

[Alan Cox]

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

Re: [PATCH] make st seekable again

[Bill Davidsen]

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 <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] make st seekable again

[Arjan van de Ven]

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

Re: [PATCH] make st seekable again

[Alan Cox]

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

Re: [PATCH] make st seekable again

[Bill Davidsen]

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 <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] make st seekable again

[Arjan van de Ven]

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. 

Re: [PATCH] make st seekable again

[Alan Cox]

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

Re: [PATCH] make st seekable again

[Bodo Eggert]

Alan Cox <alan@lxorguk.ukuu.org.uk> 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, kai.makisara@kolumbus.fi

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

Can the lseek be restricted to seek from 0 to 0 (or even * to 0 aka rewind)?
This would re-enable tar and probably other applications depending on this
API while not giving them false positives.

Re: [PATCH] make st seekable again

[Willy Tarreau]

On Thu, Mar 10, 2005 at 01:43:57AM +0100, Bodo Eggert wrote:
 
> Can the lseek be restricted to seek from 0 to 0 (or even * to 0 aka rewind)?
> This would re-enable tar and probably other applications depending on this
> API while not giving them false positives.

This would be good only if we also agree to spit out warning messages every
time the trick is used, so that people know they are relying on a buggy
application. Just like tcpdump and AF_PACKET.

willy