[PATCH] bsg: Error print if device is not bidi capable when refusing a bidi command

[PATCH] bsg: Error print if device is not bidi capable when refusing a bidi command

[Boaz Harrosh]

If a bidi command was issued to a request_queue not mark as
QUEUE_FLAG_BIDI. Issue an error report. This is a misconfiguration
an administrator would like to know about, which is otherwise
hard to detect.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 block/bsg.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/block/bsg.c b/block/bsg.c
index 44a2a0f..977547a 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -270,6 +270,8 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
 
 	if (rw == WRITE && hdr->din_xfer_len) {
 		if (!test_bit(QUEUE_FLAG_BIDI, &q->queue_flags)) {
+			printk(KERN_ERR "bsg: Attempt to send a bidi command "
+			       "to a none bidi device\n");
 			ret = -EOPNOTSUPP;
 			goto out;
 		}
-- 
1.6.0.1

Re: [PATCH] bsg: Error print if device is not bidi capable when refusing a bidi command

[Jens Axboe]

On Tue, Jan 20 2009, Boaz Harrosh wrote:
> 
> If a bidi command was issued to a request_queue not mark as
> QUEUE_FLAG_BIDI. Issue an error report. This is a misconfiguration
> an administrator would like to know about, which is otherwise
> hard to detect.

At the very least, put rate limit on that printk.

> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  block/bsg.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/block/bsg.c b/block/bsg.c
> index 44a2a0f..977547a 100644
> --- a/block/bsg.c
> +++ b/block/bsg.c
> @@ -270,6 +270,8 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
>  
>  	if (rw == WRITE && hdr->din_xfer_len) {
>  		if (!test_bit(QUEUE_FLAG_BIDI, &q->queue_flags)) {
> +			printk(KERN_ERR "bsg: Attempt to send a bidi command "
> +			       "to a none bidi device\n");
>  			ret = -EOPNOTSUPP;
>  			goto out;
>  		}
> -- 
> 1.6.0.1
> 

-- 
Jens Axboe

[PATCH ver2 printk_ratelimit] bsg: Error print if device is not bidi capable when refusing a bidi command

[Boaz Harrosh]

If a bidi command was issued to a request_queue not mark as
QUEUE_FLAG_BIDI. Issue an error report. This is a misconfiguration
an administrator would like to know about, which is otherwise
hard to detect.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 block/bsg.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/block/bsg.c b/block/bsg.c
index 44a2a0f..1da14fe 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -270,6 +270,11 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
 
 	if (rw == WRITE && hdr->din_xfer_len) {
 		if (!test_bit(QUEUE_FLAG_BIDI, &q->queue_flags)) {
+			if (printk_ratelimit()) {
+				printk(KERN_ERR
+					"bsg: Attempt to send a bidi command "
+					"to a none bidi device\n");
+			}
 			ret = -EOPNOTSUPP;
 			goto out;
 		}
-- 
1.6.0.1

Re: [PATCH] bsg: Error print if device is not bidi capable when refusing a bidi command

[Boaz Harrosh]

Jens Axboe wrote:
> On Tue, Jan 20 2009, Boaz Harrosh wrote:
>> If a bidi command was issued to a request_queue not mark as
>> QUEUE_FLAG_BIDI. Issue an error report. This is a misconfiguration
>> an administrator would like to know about, which is otherwise
>> hard to detect.
> 
> At the very least, put rate limit on that printk.
> 

Thanks, good catch it could be a DOS other wise. I did not know
of this facility. Please check if done right?

>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>> ---
>>  block/bsg.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>

Boaz

Re: [PATCH ver2 printk_ratelimit] bsg: Error print if device is not bidi capable when refusing a bidi command

[Jens Axboe]

On Tue, Jan 20 2009, Boaz Harrosh wrote:
> 
> If a bidi command was issued to a request_queue not mark as
> QUEUE_FLAG_BIDI. Issue an error report. This is a misconfiguration
> an administrator would like to know about, which is otherwise
> hard to detect.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  block/bsg.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/block/bsg.c b/block/bsg.c
> index 44a2a0f..1da14fe 100644
> --- a/block/bsg.c
> +++ b/block/bsg.c
> @@ -270,6 +270,11 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
>  
>  	if (rw == WRITE && hdr->din_xfer_len) {
>  		if (!test_bit(QUEUE_FLAG_BIDI, &q->queue_flags)) {
> +			if (printk_ratelimit()) {
> +				printk(KERN_ERR
> +					"bsg: Attempt to send a bidi command "
> +					"to a none bidi device\n");
> +			}
>  			ret = -EOPNOTSUPP;
>  			goto out;
>  		}
> -- 
> 1.6.0.1

Thanks, that works. But why isn't the -EOPNOTSUPP error return (which the
app can see and print info about) enough? Do we really need to put this
in the kernel log?

-- 
Jens Axboe

Re: [PATCH ver2 printk_ratelimit] bsg: Error print if device is not bidi capable when refusing a bidi command

[Boaz Harrosh]

Jens Axboe wrote:
> On Tue, Jan 20 2009, Boaz Harrosh wrote:
>> If a bidi command was issued to a request_queue not mark as
>> QUEUE_FLAG_BIDI. Issue an error report. This is a misconfiguration
>> an administrator would like to know about, which is otherwise
>> hard to detect.
>>
>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>> ---
>>  block/bsg.c |    5 +++++
>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/block/bsg.c b/block/bsg.c
>> index 44a2a0f..1da14fe 100644
>> --- a/block/bsg.c
>> +++ b/block/bsg.c
>> @@ -270,6 +270,11 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
>>  
>>  	if (rw == WRITE && hdr->din_xfer_len) {
>>  		if (!test_bit(QUEUE_FLAG_BIDI, &q->queue_flags)) {
>> +			if (printk_ratelimit()) {
>> +				printk(KERN_ERR
>> +					"bsg: Attempt to send a bidi command "
>> +					"to a none bidi device\n");
>> +			}
>>  			ret = -EOPNOTSUPP;
>>  			goto out;
>>  		}
>> -- 
>> 1.6.0.1
> 
> Thanks, that works. But why isn't the -EOPNOTSUPP error return (which the
> app can see and print info about) enough? Do we really need to put this
> in the kernel log?
> 

This is your call, I'm not very experienced with this things but ...

What I was thinking is in situations when things used to work fine, and
then later could stop working because of seemingly unrelated changes.
For example: The same iscsi target behind a new accelerated-iscsi-card like
cxgb3i, qla4xxx or iSER would stop working, since bidi is not enabled for
them.

But let me check I had problems with the return value. I'll try to detect
this condition from the application and report in the proper manner. Just
that I thought of the poor Admin that needs to fix that after the fact...
(me ;))

Thanks
Boaz

Re: [PATCH ver2 printk_ratelimit] bsg: Error print if device is not bidi capable when refusing a bidi command

[Boaz Harrosh]

Boaz Harrosh wrote:
> Jens Axboe wrote:
>> Thanks, that works. But why isn't the -EOPNOTSUPP error return (which the
>> app can see and print info about) enough? Do we really need to put this
>> in the kernel log?
>>
> 
> 
> But let me check I had problems with the return value.

OK I found my problem, blush:

-		ret = ioctl(fd, SG_IO, &sg);
+		ret = ioctl(fd, SG_IO, &sg) ? -errno : 0;

So if I do a print to stderr in the lowest-most library will
that be visible enough to an Admin. I have utils like
osd_format, mkfs.exofs, a FUSE filesystem ...
OK I can always redirect stderr to the dmsg log

Thanks
Boaz

Re: [PATCH ver2 printk_ratelimit] bsg: Error print if device is not bidi capable when refusing a bidi command

[FUJITA Tomonori]

On Tue, 20 Jan 2009 16:38:38 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> 
> If a bidi command was issued to a request_queue not mark as
> QUEUE_FLAG_BIDI. Issue an error report. This is a misconfiguration
> an administrator would like to know about, which is otherwise
> hard to detect.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  block/bsg.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/block/bsg.c b/block/bsg.c
> index 44a2a0f..1da14fe 100644
> --- a/block/bsg.c
> +++ b/block/bsg.c
> @@ -270,6 +270,11 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr, fmode_t has_write_perm,
>  
>  	if (rw == WRITE && hdr->din_xfer_len) {
>  		if (!test_bit(QUEUE_FLAG_BIDI, &q->queue_flags)) {
> +			if (printk_ratelimit()) {
> +				printk(KERN_ERR
> +					"bsg: Attempt to send a bidi command "
> +					"to a none bidi device\n");
> +			}
>  			ret = -EOPNOTSUPP;
>  			goto out;

I don't think that we need this patch. Returning -EOPNOTSUPP should be
enough.

Re: [PATCH ver2 printk_ratelimit] bsg: Error print if device is not bidi capable when refusing a bidi command

[Jens Axboe]

On Tue, Jan 20 2009, Boaz Harrosh wrote:
> Boaz Harrosh wrote:
> > Jens Axboe wrote:
> >> Thanks, that works. But why isn't the -EOPNOTSUPP error return (which the
> >> app can see and print info about) enough? Do we really need to put this
> >> in the kernel log?
> >>
> > 
> > 
> > But let me check I had problems with the return value.
> 
> OK I found my problem, blush:
> 
> -		ret = ioctl(fd, SG_IO, &sg);
> +		ret = ioctl(fd, SG_IO, &sg) ? -errno : 0;
> 
> So if I do a print to stderr in the lowest-most library will
> that be visible enough to an Admin. I have utils like
> osd_format, mkfs.exofs, a FUSE filesystem ...
> OK I can always redirect stderr to the dmsg log

Goodie, that saves us that printk() :-)

-- 
Jens Axboe