On 11/11/2014 06:38 PM, Elliott, Robert (Server Storage) wrote: > > >> -----Original Message----- >> From: Hannes Reinecke [mailto:hare@xxxxxxx] >> 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 <hare@xxxxxxx> >> + * >> + * 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 hare@xxxxxxx +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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
