* [PATCH] cmdfilter: extend default read filter
@ 2008-06-26 15:53 Adel Gadllah
2008-06-26 17:43 ` Jens Axboe
0 siblings, 1 reply; 4+ messages in thread
From: Adel Gadllah @ 2008-06-26 15:53 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: pjones, dgilbert, matthew, linux-scsi, jens.axboe
2008/6/26 FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>:
> On Fri, 27 Jun 2008 00:08:46 +0900
> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
>
>> On Thu, 26 Jun 2008 17:05:50 +0200
>> "Adel Gadllah" <adel.gadllah@gmail.com> wrote:
>>
>> > 2008/6/26 FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>:
>> > > On Thu, 26 Jun 2008 12:10:25 +0200
>> > > "Adel Gadllah" <adel.gadllah@gmail.com> wrote:
>> > >
>> > >> 2008/6/18 Peter Jones <pjones@redhat.com>:
>> > >> > Douglas Gilbert wrote:
>> > >> >>
>> > >> >> Peter Jones wrote:
>> > >> >>>
>> > >> >>> FUJITA Tomonori wrote:
>> > >> >>>
>> > >> >>>> Well, this changes sg behaviour since sg's allow_ops filter has a
>> > >> >>>> access permission different from blk_verify_command filter's.
>> > >> >>>
>> > >> >>> >
>> > >> >>>>
>> > >> >>>> I guess that the first thing you need to do is that figuring out a
>> > >> >>>> proper access permission for each command, which sg maintainer, etc
>> > >> >>>> can agree. It's pretty hard and that's the reason why this patch has
>> > >> >>>> not been merged for years, I think.
>> > >> >>>
>> > >> >>> I don't think this logic is sound.
>> > >> >>
>> > >> >> That depends on your viewpoint.
>> > >> >
>> > >> > My viewpoint is this:
>> > >> >
>> > >> > 1) Whether you agree with his reasons or not, Linus made it pretty clear
>> > >> > that he's against removing the command filter (see
>> > >> > http://marc.info/?l=linux-scsi&m=115419945212450&w=2 )
>> > >> > 2) Having different code paths use different filtering code just adds more
>> > >> > confusion.
>> > >> > 3) If we're going to have filtering, it should be configurable on a
>> > >> > per-device basis from userland.
>> > >> >
>> > >> > Which of these do you disagree with?
>> > >> >
>> > >> > [...]
>> > >> >>
>> > >> >> Are per device command filters being proposed?
>> > >> >
>> > >> > Yes, that's what the patch implements. And it allows the userland to
>> > >> > configure them according to the needs of the hardware.
>> > >>
>> > >> Jens can we add merge this for .27 or does anyone still has objections?
>> > >
>> > > I think that this patch makes sg's permission stricter. So this could
>> > > break the existing user-space applications.
>> > >
>> >
>> > any particular app in mind?
>>
>> No, but there would be some.
>>
>>
>> > for write access it still allows all commands (because there are some
>> > userspace apps tha rely on this).
>>
>> Yeah, I know. But for read access, some commands will be blocked.
>
> I think that it's not a good idea to say "this patch could break
> something but we have no idea about them. So we can merge this."
>
> It's better to loosen scsi_ioctl's permissions to match with sg's
> permission.
agreed.
Jens please apply the attached patch on top of the old one.
---------------------------
This patch adds the commands that the former sg filter allowed for read
access to the cmdfilter to keep userspace apps that rely on them working.
Signed-off-by: Adel Gadllah <adel.gadllah@gmail.com>
diff -upNr linux-2.6.orign/block/cmd-filter.c linux-2.6/block/cmd-filter.c
--- linux-2.6.orign/block/cmd-filter.c 2008-06-26 17:41:20.000000000 +0200
+++ linux-2.6/block/cmd-filter.c 2008-06-26 17:43:23.000000000 +0200
@@ -230,6 +230,7 @@ static void rcf_set_defaults(struct blk_
__set_bit(READ_16, filter->read_ok);
__set_bit(READ_BUFFER, filter->read_ok);
__set_bit(READ_DEFECT_DATA, filter->read_ok);
+ __set_bit(READ_CAPACITY, filter->read_ok);
__set_bit(READ_LONG, filter->read_ok);
__set_bit(INQUIRY, filter->read_ok);
__set_bit(MODE_SENSE, filter->read_ok);
@@ -238,6 +239,10 @@ static void rcf_set_defaults(struct blk_
__set_bit(START_STOP, filter->read_ok);
__set_bit(GPCMD_VERIFY_10, filter->read_ok);
__set_bit(VERIFY_16, filter->read_ok);
+ __set_bit(REPORT_LUNS, filter->read_ok);
+ __set_bit(SERVICE_ACTION_IN, filter->read_ok);
+ __set_bit(RECEIVE_DIAGNOSTIC, filter->read_ok);
+ __set_bit(MAINTENANCE_IN_CMD, filter->read_ok);
__set_bit(GPCMD_READ_BUFFER_CAPACITY, filter->read_ok);
/* Audio CD commands */
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] cmdfilter: extend default read filter
2008-06-26 15:53 [PATCH] cmdfilter: extend default read filter Adel Gadllah
@ 2008-06-26 17:43 ` Jens Axboe
2008-06-26 18:04 ` Adel Gadllah
0 siblings, 1 reply; 4+ messages in thread
From: Jens Axboe @ 2008-06-26 17:43 UTC (permalink / raw)
To: Adel Gadllah; +Cc: FUJITA Tomonori, pjones, dgilbert, matthew, linux-scsi
On Thu, Jun 26 2008, Adel Gadllah wrote:
> 2008/6/26 FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>:
> > On Fri, 27 Jun 2008 00:08:46 +0900
> > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> >
> >> On Thu, 26 Jun 2008 17:05:50 +0200
> >> "Adel Gadllah" <adel.gadllah@gmail.com> wrote:
> >>
> >> > 2008/6/26 FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>:
> >> > > On Thu, 26 Jun 2008 12:10:25 +0200
> >> > > "Adel Gadllah" <adel.gadllah@gmail.com> wrote:
> >> > >
> >> > >> 2008/6/18 Peter Jones <pjones@redhat.com>:
> >> > >> > Douglas Gilbert wrote:
> >> > >> >>
> >> > >> >> Peter Jones wrote:
> >> > >> >>>
> >> > >> >>> FUJITA Tomonori wrote:
> >> > >> >>>
> >> > >> >>>> Well, this changes sg behaviour since sg's allow_ops filter has a
> >> > >> >>>> access permission different from blk_verify_command filter's.
> >> > >> >>>
> >> > >> >>> >
> >> > >> >>>>
> >> > >> >>>> I guess that the first thing you need to do is that figuring out a
> >> > >> >>>> proper access permission for each command, which sg maintainer, etc
> >> > >> >>>> can agree. It's pretty hard and that's the reason why this patch has
> >> > >> >>>> not been merged for years, I think.
> >> > >> >>>
> >> > >> >>> I don't think this logic is sound.
> >> > >> >>
> >> > >> >> That depends on your viewpoint.
> >> > >> >
> >> > >> > My viewpoint is this:
> >> > >> >
> >> > >> > 1) Whether you agree with his reasons or not, Linus made it pretty clear
> >> > >> > that he's against removing the command filter (see
> >> > >> > http://marc.info/?l=linux-scsi&m=115419945212450&w=2 )
> >> > >> > 2) Having different code paths use different filtering code just adds more
> >> > >> > confusion.
> >> > >> > 3) If we're going to have filtering, it should be configurable on a
> >> > >> > per-device basis from userland.
> >> > >> >
> >> > >> > Which of these do you disagree with?
> >> > >> >
> >> > >> > [...]
> >> > >> >>
> >> > >> >> Are per device command filters being proposed?
> >> > >> >
> >> > >> > Yes, that's what the patch implements. And it allows the userland to
> >> > >> > configure them according to the needs of the hardware.
> >> > >>
> >> > >> Jens can we add merge this for .27 or does anyone still has objections?
> >> > >
> >> > > I think that this patch makes sg's permission stricter. So this could
> >> > > break the existing user-space applications.
> >> > >
> >> >
> >> > any particular app in mind?
> >>
> >> No, but there would be some.
> >>
> >>
> >> > for write access it still allows all commands (because there are some
> >> > userspace apps tha rely on this).
> >>
> >> Yeah, I know. But for read access, some commands will be blocked.
> >
> > I think that it's not a good idea to say "this patch could break
> > something but we have no idea about them. So we can merge this."
> >
> > It's better to loosen scsi_ioctl's permissions to match with sg's
> > permission.
>
> agreed.
>
> Jens please apply the attached patch on top of the old one.
Thanks, I thought this had already been worked out when I saw the
previous discussion. Please verify that this patch then no longer causes
ANY different behaviour than what we already have, that's a prerequisite
for merging it.
I've applied this patch.
--
Jens Axboe
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] cmdfilter: extend default read filter
2008-06-26 17:43 ` Jens Axboe
@ 2008-06-26 18:04 ` Adel Gadllah
2008-06-26 18:06 ` Jens Axboe
0 siblings, 1 reply; 4+ messages in thread
From: Adel Gadllah @ 2008-06-26 18:04 UTC (permalink / raw)
To: Jens Axboe; +Cc: FUJITA Tomonori, pjones, dgilbert, matthew, linux-scsi
2008/6/26 Jens Axboe <jens.axboe@oracle.com>:
> On Thu, Jun 26 2008, Adel Gadllah wrote:
>> 2008/6/26 FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>:
>> > On Fri, 27 Jun 2008 00:08:46 +0900
>> > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
>> >
>> >> On Thu, 26 Jun 2008 17:05:50 +0200
>> >> "Adel Gadllah" <adel.gadllah@gmail.com> wrote:
>> >>
>> >> > 2008/6/26 FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>:
>> >> > > On Thu, 26 Jun 2008 12:10:25 +0200
>> >> > > "Adel Gadllah" <adel.gadllah@gmail.com> wrote:
>> >> > >
>> >> > >> 2008/6/18 Peter Jones <pjones@redhat.com>:
>> >> > >> > Douglas Gilbert wrote:
>> >> > >> >>
>> >> > >> >> Peter Jones wrote:
>> >> > >> >>>
>> >> > >> >>> FUJITA Tomonori wrote:
>> >> > >> >>>
>> >> > >> >>>> Well, this changes sg behaviour since sg's allow_ops filter has a
>> >> > >> >>>> access permission different from blk_verify_command filter's.
>> >> > >> >>>
>> >> > >> >>> >
>> >> > >> >>>>
>> >> > >> >>>> I guess that the first thing you need to do is that figuring out a
>> >> > >> >>>> proper access permission for each command, which sg maintainer, etc
>> >> > >> >>>> can agree. It's pretty hard and that's the reason why this patch has
>> >> > >> >>>> not been merged for years, I think.
>> >> > >> >>>
>> >> > >> >>> I don't think this logic is sound.
>> >> > >> >>
>> >> > >> >> That depends on your viewpoint.
>> >> > >> >
>> >> > >> > My viewpoint is this:
>> >> > >> >
>> >> > >> > 1) Whether you agree with his reasons or not, Linus made it pretty clear
>> >> > >> > that he's against removing the command filter (see
>> >> > >> > http://marc.info/?l=linux-scsi&m=115419945212450&w=2 )
>> >> > >> > 2) Having different code paths use different filtering code just adds more
>> >> > >> > confusion.
>> >> > >> > 3) If we're going to have filtering, it should be configurable on a
>> >> > >> > per-device basis from userland.
>> >> > >> >
>> >> > >> > Which of these do you disagree with?
>> >> > >> >
>> >> > >> > [...]
>> >> > >> >>
>> >> > >> >> Are per device command filters being proposed?
>> >> > >> >
>> >> > >> > Yes, that's what the patch implements. And it allows the userland to
>> >> > >> > configure them according to the needs of the hardware.
>> >> > >>
>> >> > >> Jens can we add merge this for .27 or does anyone still has objections?
>> >> > >
>> >> > > I think that this patch makes sg's permission stricter. So this could
>> >> > > break the existing user-space applications.
>> >> > >
>> >> >
>> >> > any particular app in mind?
>> >>
>> >> No, but there would be some.
>> >>
>> >>
>> >> > for write access it still allows all commands (because there are some
>> >> > userspace apps tha rely on this).
>> >>
>> >> Yeah, I know. But for read access, some commands will be blocked.
>> >
>> > I think that it's not a good idea to say "this patch could break
>> > something but we have no idea about them. So we can merge this."
>> >
>> > It's better to loosen scsi_ioctl's permissions to match with sg's
>> > permission.
>>
>> agreed.
>>
>> Jens please apply the attached patch on top of the old one.
>
> Thanks, I thought this had already been worked out when I saw the
> previous discussion. Please verify that this patch then no longer causes
> ANY different behaviour than what we already have, that's a prerequisite
> for merging it.
With this patch all apps that worked with the old scsi_ioctl, bsg and
sg filter should behave exactly the same as without the new filter
infrastructure. (it adds no new restrictions)
The only thing it adds is that it allows READ_CAPACITY, REPORT_LUNS,
SERVICE_ACTION_IN, RECEIVE_DIAGNOSTIC and MAINTENANCE_IN_CMD for bsg
and scsi_ioctl in read only mode.
As this was already allowed using /dev/sg* the commands should be
safe, so no regression added.
> I've applied this patch.
Thanks,
Adel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] cmdfilter: extend default read filter
2008-06-26 18:04 ` Adel Gadllah
@ 2008-06-26 18:06 ` Jens Axboe
0 siblings, 0 replies; 4+ messages in thread
From: Jens Axboe @ 2008-06-26 18:06 UTC (permalink / raw)
To: Adel Gadllah; +Cc: FUJITA Tomonori, pjones, dgilbert, matthew, linux-scsi
On Thu, Jun 26 2008, Adel Gadllah wrote:
> 2008/6/26 Jens Axboe <jens.axboe@oracle.com>:
> > On Thu, Jun 26 2008, Adel Gadllah wrote:
> >> 2008/6/26 FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>:
> >> > On Fri, 27 Jun 2008 00:08:46 +0900
> >> > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> >> >
> >> >> On Thu, 26 Jun 2008 17:05:50 +0200
> >> >> "Adel Gadllah" <adel.gadllah@gmail.com> wrote:
> >> >>
> >> >> > 2008/6/26 FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>:
> >> >> > > On Thu, 26 Jun 2008 12:10:25 +0200
> >> >> > > "Adel Gadllah" <adel.gadllah@gmail.com> wrote:
> >> >> > >
> >> >> > >> 2008/6/18 Peter Jones <pjones@redhat.com>:
> >> >> > >> > Douglas Gilbert wrote:
> >> >> > >> >>
> >> >> > >> >> Peter Jones wrote:
> >> >> > >> >>>
> >> >> > >> >>> FUJITA Tomonori wrote:
> >> >> > >> >>>
> >> >> > >> >>>> Well, this changes sg behaviour since sg's allow_ops filter has a
> >> >> > >> >>>> access permission different from blk_verify_command filter's.
> >> >> > >> >>>
> >> >> > >> >>> >
> >> >> > >> >>>>
> >> >> > >> >>>> I guess that the first thing you need to do is that figuring out a
> >> >> > >> >>>> proper access permission for each command, which sg maintainer, etc
> >> >> > >> >>>> can agree. It's pretty hard and that's the reason why this patch has
> >> >> > >> >>>> not been merged for years, I think.
> >> >> > >> >>>
> >> >> > >> >>> I don't think this logic is sound.
> >> >> > >> >>
> >> >> > >> >> That depends on your viewpoint.
> >> >> > >> >
> >> >> > >> > My viewpoint is this:
> >> >> > >> >
> >> >> > >> > 1) Whether you agree with his reasons or not, Linus made it pretty clear
> >> >> > >> > that he's against removing the command filter (see
> >> >> > >> > http://marc.info/?l=linux-scsi&m=115419945212450&w=2 )
> >> >> > >> > 2) Having different code paths use different filtering code just adds more
> >> >> > >> > confusion.
> >> >> > >> > 3) If we're going to have filtering, it should be configurable on a
> >> >> > >> > per-device basis from userland.
> >> >> > >> >
> >> >> > >> > Which of these do you disagree with?
> >> >> > >> >
> >> >> > >> > [...]
> >> >> > >> >>
> >> >> > >> >> Are per device command filters being proposed?
> >> >> > >> >
> >> >> > >> > Yes, that's what the patch implements. And it allows the userland to
> >> >> > >> > configure them according to the needs of the hardware.
> >> >> > >>
> >> >> > >> Jens can we add merge this for .27 or does anyone still has objections?
> >> >> > >
> >> >> > > I think that this patch makes sg's permission stricter. So this could
> >> >> > > break the existing user-space applications.
> >> >> > >
> >> >> >
> >> >> > any particular app in mind?
> >> >>
> >> >> No, but there would be some.
> >> >>
> >> >>
> >> >> > for write access it still allows all commands (because there are some
> >> >> > userspace apps tha rely on this).
> >> >>
> >> >> Yeah, I know. But for read access, some commands will be blocked.
> >> >
> >> > I think that it's not a good idea to say "this patch could break
> >> > something but we have no idea about them. So we can merge this."
> >> >
> >> > It's better to loosen scsi_ioctl's permissions to match with sg's
> >> > permission.
> >>
> >> agreed.
> >>
> >> Jens please apply the attached patch on top of the old one.
> >
> > Thanks, I thought this had already been worked out when I saw the
> > previous discussion. Please verify that this patch then no longer causes
> > ANY different behaviour than what we already have, that's a prerequisite
> > for merging it.
>
> With this patch all apps that worked with the old scsi_ioctl, bsg and
> sg filter should behave exactly the same as without the new filter
> infrastructure. (it adds no new restrictions)
> The only thing it adds is that it allows READ_CAPACITY, REPORT_LUNS,
> SERVICE_ACTION_IN, RECEIVE_DIAGNOSTIC and MAINTENANCE_IN_CMD for bsg
> and scsi_ioctl in read only mode.
> As this was already allowed using /dev/sg* the commands should be
> safe, so no regression added.
Alright, I think that is good enough. Thanks.
--
Jens Axboe
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-06-26 18:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-06-26 15:53 [PATCH] cmdfilter: extend default read filter Adel Gadllah
2008-06-26 17:43 ` Jens Axboe
2008-06-26 18:04 ` Adel Gadllah
2008-06-26 18:06 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).