| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <da824cf30903250904n211b157eje2db99b0bdb1f33a@mail.gmail.com>
Date: Wed, 25 Mar 2009 09:04:04 -0700
From: Grant Grundler <grundler@...gle.com>
To: Tejun Heo <tj@...nel.org>
Cc: bzolnier@...il.com, linux-kernel@...r.kernel.org, axboe@...nel.dk,
linux-ide@...r.kernel.org
Subject: Re: [PATCH 02/10] ide-tape: use single continuous buffer
On Wed, Mar 25, 2009 at 7:17 AM, Tejun Heo <tj@...nel.org> wrote:
> Impact: simpler buffer allocation and handling, fix DMA transfers
...
> + atomic_set(&bh->b_count, bcount);
> if (atomic_read(&bh->b_count) == bh->b_size)
...
I'm failing to see why bh->b_count is an atomic_t.
I always assumed tapes were exclusive access devices
and would be serialized at a higher level.
> /*
> - * The function below uses __get_free_pages to allocate a data buffer of size
> - * tape->buffer_size (or a bit more). We attempt to combine sequential pages as
> - * much as possible.
> - *
> - * It returns a pointer to the newly allocated buffer, or NULL in case of
> - * failure.
> + * It returns a pointer to the newly allocated buffer, or NULL in case
> + * of failure.
> */
> static struct idetape_bh *ide_tape_kmalloc_buffer(idetape_tape_t *tape,
> - int full, int clear)
> -{
...
> -abort:
> - ide_tape_kfree_buffer(tape);
> - return NULL;
> + bh->b_size = tape->buffer_size;
> + atomic_set(&bh->b_count, full ? bh->b_size : 0);
No one else could possibly be referencing bh->count at this
point...I like that it's consistent though.
The use of atomic won't hurt correctness and this patch looks fine to me.
Please add "Reviewed-by: Grant Grundler <grundler@...gle.com>"
thanks,
grant
...
> @@ -977,30 +872,20 @@ static int idetape_copy_stage_to_user(idetape_tape_t *tape, char __user *buf,
> int n)
> {
> struct idetape_bh *bh = tape->bh;
> - int count;
> int ret = 0;
>
> - while (n) {
> - if (bh == NULL) {
> + if (n) {
> + if (bh == NULL || n > tape->b_count) {
> printk(KERN_ERR "ide-tape: bh == NULL in %s\n",
> __func__);
> return 1;
> }
> - count = min(tape->b_count, n);
> - if (copy_to_user(buf, tape->b_data, count))
> + if (copy_to_user(buf, tape->b_data, n))
> ret = 1;
> - n -= count;
> - tape->b_data += count;
> - tape->b_count -= count;
> - buf += count;
> - if (!tape->b_count) {
> - bh = bh->b_reqnext;
> - tape->bh = bh;
> - if (bh) {
> - tape->b_data = bh->b_data;
> - tape->b_count = atomic_read(&bh->b_count);
> - }
> - }
> + tape->b_data += n;
> + tape->b_count -= n;
> + if (!tape->b_count)
> + tape->bh = NULL;
> }
> return ret;
> }
> @@ -1252,7 +1137,7 @@ static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks)
> static void ide_tape_flush_merge_buffer(ide_drive_t *drive)
> {
> idetape_tape_t *tape = drive->driver_data;
> - int blocks, min;
> + int blocks;
> struct idetape_bh *bh;
>
> if (tape->chrdev_dir != IDETAPE_DIR_WRITE) {
> @@ -1267,31 +1152,16 @@ static void ide_tape_flush_merge_buffer(ide_drive_t *drive)
> if (tape->merge_bh_size) {
> blocks = tape->merge_bh_size / tape->blk_size;
> if (tape->merge_bh_size % tape->blk_size) {
> - unsigned int i;
> -
> + unsigned int i = tape->blk_size -
> + tape->merge_bh_size % tape->blk_size;
> blocks++;
> - i = tape->blk_size - tape->merge_bh_size %
> - tape->blk_size;
> - bh = tape->bh->b_reqnext;
> - while (bh) {
> - atomic_set(&bh->b_count, 0);
> - bh = bh->b_reqnext;
> - }
> bh = tape->bh;
> - while (i) {
> - if (bh == NULL) {
> - printk(KERN_INFO "ide-tape: bug,"
> - " bh NULL\n");
> - break;
> - }
> - min = min(i, (unsigned int)(bh->b_size -
> - atomic_read(&bh->b_count)));
> + if (bh) {
> memset(bh->b_data + atomic_read(&bh->b_count),
> - 0, min);
> - atomic_add(min, &bh->b_count);
> - i -= min;
> - bh = bh->b_reqnext;
> - }
> + 0, i);
> + atomic_add(i, &bh->b_count);
> + } else
> + printk(KERN_INFO "ide-tape: bug, bh NULL\n");
> }
> (void) idetape_add_chrdev_write_request(drive, blocks);
> tape->merge_bh_size = 0;
> @@ -1319,7 +1189,7 @@ static int idetape_init_read(ide_drive_t *drive)
> " 0 now\n");
> tape->merge_bh_size = 0;
> }
> - tape->merge_bh = ide_tape_kmalloc_buffer(tape, 0, 0);
> + tape->merge_bh = ide_tape_kmalloc_buffer(tape, 0);
> if (!tape->merge_bh)
> return -ENOMEM;
> tape->chrdev_dir = IDETAPE_DIR_READ;
> @@ -1366,23 +1236,18 @@ static int idetape_add_chrdev_read_request(ide_drive_t *drive, int blocks)
> static void idetape_pad_zeros(ide_drive_t *drive, int bcount)
> {
> idetape_tape_t *tape = drive->driver_data;
> - struct idetape_bh *bh;
> + struct idetape_bh *bh = tape->merge_bh;
> int blocks;
>
> while (bcount) {
> unsigned int count;
>
> - bh = tape->merge_bh;
> count = min(tape->buffer_size, bcount);
> bcount -= count;
> blocks = count / tape->blk_size;
> - while (count) {
> - atomic_set(&bh->b_count,
> - min(count, (unsigned int)bh->b_size));
> - memset(bh->b_data, 0, atomic_read(&bh->b_count));
> - count -= atomic_read(&bh->b_count);
> - bh = bh->b_reqnext;
> - }
> + atomic_set(&bh->b_count, count);
> + memset(bh->b_data, 0, atomic_read(&bh->b_count));
> +
> idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE, blocks,
> tape->merge_bh);
> }
> @@ -1594,7 +1459,7 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
> "should be 0 now\n");
> tape->merge_bh_size = 0;
> }
> - tape->merge_bh = ide_tape_kmalloc_buffer(tape, 0, 0);
> + tape->merge_bh = ide_tape_kmalloc_buffer(tape, 0);
> if (!tape->merge_bh)
> return -ENOMEM;
> tape->chrdev_dir = IDETAPE_DIR_WRITE;
> @@ -1968,7 +1833,7 @@ static void idetape_write_release(ide_drive_t *drive, unsigned int minor)
> idetape_tape_t *tape = drive->driver_data;
>
> ide_tape_flush_merge_buffer(drive);
> - tape->merge_bh = ide_tape_kmalloc_buffer(tape, 1, 0);
> + tape->merge_bh = ide_tape_kmalloc_buffer(tape, 1);
> if (tape->merge_bh != NULL) {
> idetape_pad_zeros(drive, tape->blk_size *
> (tape->user_bs_factor - 1));
> @@ -2199,11 +2064,6 @@ static void idetape_setup(ide_drive_t *drive, idetape_tape_t *tape, int minor)
> tape->buffer_size = *ctl * tape->blk_size;
> }
> buffer_size = tape->buffer_size;
> - tape->pages_per_buffer = buffer_size / PAGE_SIZE;
> - if (buffer_size % PAGE_SIZE) {
> - tape->pages_per_buffer++;
> - tape->excess_bh_size = PAGE_SIZE - buffer_size % PAGE_SIZE;
> - }
>
> /* select the "best" DSC read/write polling freq */
> speed = max(*(u16 *)&tape->caps[14], *(u16 *)&tape->caps[8]);
> --
> 1.6.0.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Powered by blists - more mailing lists