On 11/11/2014 06:38 PM, Elliott, Robert (Server Storage) wrote:
>
>
>> -----Original Message-----
>> From: Hannes Reinecke [mailto:h...@suse.de]
>> Sent: Tuesday, 04 November, 2014 2:07 AM
> ...
>> diff --git a/drivers/scsi/scsi_logging.c
>> b/drivers/scsi/scsi_logging.c
> ...
>> @@ -0,0 +1,119 @@
>> +/*
>> + * scsi_logging.c
>> + *
>> + * Copyright (C) 2014 SUSE Linux Products GmbH
>> + * Copyright (C) 2014 Hannes Reinecke <h...@suse.de>
>> + *
>> + * This file is released under the GPLv2
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/atomic.h>
>> +
>> +#include <scsi/scsi.h>
>> +#include <scsi/scsi_cmnd.h>
>> +#include <scsi/scsi_device.h>
>> +#include <scsi/scsi_dbg.h>
>> +
>> +#define SCSI_LOG_SPOOLSIZE 4096
>> +#define SCSI_LOG_BUFSIZE 128
>> +
>> +struct scsi_log_buf {
>> + char buffer[SCSI_LOG_SPOOLSIZE];
>> + unsigned long map;
>> +};
>> +
>> +static DEFINE_PER_CPU(struct scsi_log_buf, scsi_format_log);
>> +
>> +static char *scsi_log_reserve_buffer(size_t *len)
>> +{
>> + struct scsi_log_buf *buf;
>> + unsigned long map_bits = SCSI_LOG_SPOOLSIZE / SCSI_LOG_BUFSIZE;
>> + unsigned long idx = 0;
>> +
>> + WARN_ON(map_bits > BITS_PER_LONG);
>
> Since SCSI_LOG_SPOOLSIZE, SCSI_LOG_BUFSIZE, and BITS_PER_LONG
> are constants, that can be a compile-time check.
>
Ok.
>> + preempt_disable();
>> + buf = this_cpu_ptr(&scsi_format_log);
>> + idx = find_first_zero_bit(&buf->map, map_bits);
>
> If this fails to find a bit, it returns map_bits.
> This could result in the next test_and_set_bit call
> accessing an address that is outside the bounds of
> buf->map.
>
> A safety check seems prudent before the test_and_set_bit:
> if (likely(idx < map_bits))
>
Ah. I wasn't quite sure what find_first_zero_bit would return
on failure. But yeah, that check seems to be appropriate.
>> + while (test_and_set_bit(idx, &buf->map)) {
>> + idx = find_next_zero_bit(&buf->map, map_bits, idx);
>> + if (idx >= map_bits) {
>> + break;
>> + }
>
> scripts/checkpatch.pl -f doesn't like the {} on that.
>
Sigh.
>> + }
>> + if (WARN_ON(idx >= map_bits)) {
>> + preempt_enable();
>> + return NULL;
>> + }
>> + *len = SCSI_LOG_BUFSIZE;
>> + return buf->buffer + idx * SCSI_LOG_BUFSIZE;
>> +}
>> +
>> +static void scsi_log_release_buffer(char *bufptr)
>> +{
>> + struct scsi_log_buf *buf;
>> + unsigned long idx;
>> + int ret;
>> +
>> + buf = this_cpu_ptr(&scsi_format_log);
>> + if (bufptr < buf->buffer + SCSI_LOG_SPOOLSIZE) {
>
> Should that also check that bufptr > buf->buffer?
>
Probably.
>> + idx = (bufptr - buf->buffer) / SCSI_LOG_BUFSIZE;
>> + ret = test_and_clear_bit(idx, &buf->map);
>> + WARN_ON(!ret);
>> + }
>> + preempt_enable();
>> +}
>> +
>> +int sdev_prefix_printk(const char *level, const struct scsi_device
>> *sdev,
>> + const char *name, const char *fmt, ...)
>> +{
>> + va_list args;
>> + char *logbuf;
>> + size_t off = 0, logbuf_len;
>> + int ret;
>> +
>> + if (!sdev)
>> + return 0;
>> +
>> + logbuf = scsi_log_reserve_buffer(&logbuf_len);
>> + if (!logbuf)
>> + return 0;
>> +
>> + if (name)
>> + off += scnprintf(logbuf + off, logbuf_len - off,
>> + "[%s] ", name);
>> + va_start(args, fmt);
>> + off += vscnprintf(logbuf + off, logbuf_len - off, fmt, args);
>> + va_end(args);
>> + ret = dev_printk(level, &sdev->sdev_gendev, "%s", logbuf);
>> + scsi_log_release_buffer(logbuf);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(sdev_prefix_printk);
>> +
>> +int scmd_printk(const char *level, const struct scsi_cmnd *scmd,
>> + const char *fmt, ...)
>> +{
>> + struct gendisk *disk = scmd->request->rq_disk;
>> + va_list args;
>> + char *logbuf;
>> + size_t off = 0, logbuf_len;
>> + int ret;
>> +
>> + if (!scmd || scmd->cmnd == NULL)
>> + return 0;
>
> !scmd->cmnd seems more common in neighboring code.
>
Ok.
>> +
>> + logbuf = scsi_log_reserve_buffer(&logbuf_len);
>> + if (!logbuf)
>> + return 0;
>> + if (disk)
>> + off += scnprintf(logbuf + off, logbuf_len - off,
>> + "[%s] ", disk->disk_name);
>> + va_start(args, fmt);
>> + off += vscnprintf(logbuf + off, logbuf_len - off, fmt, args);
>> + va_end(args);
>> + ret = dev_printk(level, &scmd->device->sdev_gendev, "%s",
>> logbuf);
>> + scsi_log_release_buffer(logbuf);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(scmd_printk);
> ...
>
> dev_printk is EXPORT_SYMBOL. The former macro versions of
> sdev_printk and scmd_printk just called dev_printk, and not
> being symbols, did not change that.
>
> So, I think these function versions should use EXPORT_SYMBOL, so
> the hated binary drivers can still call them.
>
Okay, will be fixing things up for the next revision.
Thanks for the review.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
