linux-kernel - Re: [PATCH] Fix setting bio flags in drivers (sd

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFR8uecR0+dYeY47ztGvg02uCPXv8TTi_cbaBqc+Cq-v=NMe4Q@mail.gmail.com>
Date:	Thu, 1 Mar 2012 13:20:51 -0800
From:	Muthu Kumar <muthu.lkml@...il.com>
To:	Jens Axboe <axboe@...nel.dk>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, torvalds@...ux-foundation.org,
	martin.petersen@...cle.com
Subject: Re: [PATCH] Fix setting bio flags in drivers (sd_dif/floppy).

On Thu, Mar 1, 2012 at 12:25 PM, Jens Axboe <axboe@...nel.dk> wrote:
> On 2012-03-01 21:23, Muthu Kumar wrote:
>>>>                       kunmap_atomic(sdt, KM_USER0);
>>>>               }
>>>>
>>>> -             bio->bi_flags |= BIO_MAPPED_INTEGRITY;
>>>> +             bio->bi_flags |= (1 << BIO_MAPPED_INTEGRITY);
>>>>       }
>>>>
>>>>       return 0;
>>>
>>> urgh.  This isn't the first time.
>>>
>>> It's too easy for people to make this mistake.  I'm not sure what a
>>> good fix would be - I don't think sparse can save us with __bitwise or
>>> similar.
>>>
>>> The approach we took in buffer_head.h with BH_Foo and BUFFER_FNS
>>> accessors worked pretty well.
>>>
>>
>> Does this look good? BTW, I used the non-atomic variants __set/__clear
>> to match the behavior of current usage.
>> If this looks good, I can send the proper patch as attachment (so no
>> line wraps :)
>
> Lets not wrap this in macros, it just makes the code harder to read.
> Making the BIO_ flags a bit shift value was a mistake in hindsight, too
> easy to get wrong.
>
> If we're going to be changing everything anyway, it'd be better to have
> __ variants if we really need the bit shifts, and the non __ variants be
> the value. Similar to what is done for request flags.
>
> --
> Jens Axboe
>


You mean, like this? (sparing the gmail line wrap)

diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 4053cbd..035e1ef 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -83,19 +83,35 @@ struct bio {
 /*
  * bio flags
  */
-#define BIO_UPTODATE   0       /* ok after I/O completion */
-#define BIO_RW_BLOCK   1       /* RW_AHEAD set, and read/write would block */
-#define BIO_EOF                2       /* out-out-bounds error */
-#define BIO_SEG_VALID  3       /* bi_phys_segments valid */
-#define BIO_CLONED     4       /* doesn't own data */
-#define BIO_BOUNCED    5       /* bio is a bounce bio */
-#define BIO_USER_MAPPED 6      /* contains user pages */
-#define BIO_EOPNOTSUPP 7       /* not supported */
-#define BIO_NULL_MAPPED 8      /* contains invalid user pages */
-#define BIO_FS_INTEGRITY 9     /* fs owns integrity data, not block layer */
-#define BIO_QUIET      10      /* Make BIO Quiet */
-#define BIO_MAPPED_INTEGRITY 11/* integrity metadata has been remapped */
-#define bio_flagged(bio, flag) ((bio)->bi_flags & (1 << (flag)))
+enum bio_flag_bits {
+    __BIO_UPTODATE,             /* ok after I/O completion */
+    __BIO_RW_BLOCK,             /* RW_AHEAD set, and read/write would block */
+    __BIO_EOF,                  /* out-out-bounds error */
+    __BIO_SEG_VALID,            /* bi_phys_segments valid */
+    __BIO_CLONED,               /* doesn't own data */
+    __BIO_BOUNCED,              /* bio is a bounce bio */
+    __BIO_USER_MAPPED,          /* contains user pages */
+    __BIO_EOPNOTSUPP,           /* not supported */
+    __BIO_NULL_MAPPED,          /* contains invalid user pages */
+    __BIO_FS_INTEGRITY,         /* fs owns integrity data, not block layer */
+    __BIO_QUIET,                /* Make BIO Quiet */
+    __BIO_MAPPED_INTEGRITY,     /* integrity metadata has been remapped */
+};
+
+#define BIO_UPTODATE            (1 << __BIO_UPTODATE)
+#define BIO_RW_BLOCK            (1 << __BIO_RW_BLOCK)
+#define BIO_EOF                 (1 << __BIO_EOF)
+#define BIO_SEG_VALID           (1 << __BIO_SEG_VALID)
+#define BIO_CLONED              (1 << __BIO_CLONED)
+#define BIO_BOUNCED             (1 << __BIO_BOUNCED)
+#define BIO_USER_MAPPED         (1 << __BIO_USER_MAPPED)
+#define BIO_EOPNOTSUPP          (1 << __BIO_EOPNOTSUPP)
+#define BIO_NULL_MAPPED         (1 << __BIO_NULL_MAPPED)
+#define BIO_FS_INTEGRITY        (1 << __BIO_FS_INTEGRITY)
+#define BIO_QUIET               (1 << __BIO_QUIET)
+#define BIO_MAPPED_INTEGRITY    (1 << __BIO_MAPPED_INTEGRITY)
+
+#define bio_flagged(bio, flag) ((bio)->bi_flags & (flag))

 /*
  * top 4 bits of bio flags indicate the pool this bio came from
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/