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: <CAFdhcLQ+LrVyqeBeXf++sV2RddBn2Rn6w7ZRX1szU+XW8+SPXA@mail.gmail.com> Date: Thu, 14 Aug 2014 16:07:39 -0400 From: David Horner <ds2horner@...il.com> To: Dan Streetman <ddstreet@...e.org> Cc: Minchan Kim <minchan@...nel.org>, Andrew Morton <akpm@...ux-foundation.org>, Linux-MM <linux-mm@...ck.org>, linux-kernel <linux-kernel@...r.kernel.org>, Sergey Senozhatsky <sergey.senozhatsky@...il.com>, Jerome Marchand <jmarchan@...hat.com>, juno.choi@....com, seungho1.park@....com, Luigi Semenzato <semenzato@...gle.com>, Nitin Gupta <ngupta@...are.org>, Seth Jennings <sjennings@...iantweb.net> Subject: Re: [PATCH 3/3] zram: add mem_used_max via sysfs On Thu, Aug 14, 2014 at 3:11 PM, Dan Streetman <ddstreet@...e.org> wrote: > On Thu, Aug 14, 2014 at 12:23 PM, David Horner <ds2horner@...il.com> wrote: >> On Thu, Aug 14, 2014 at 11:32 AM, David Horner <ds2horner@...il.com> wrote: >>> On Thu, Aug 14, 2014 at 11:09 AM, Dan Streetman <ddstreet@...e.org> wrote: >>>> On Wed, Aug 13, 2014 at 9:12 PM, Minchan Kim <minchan@...nel.org> wrote: >>>>> - if (zram->limit_bytes && >>>>> - zs_get_total_size_bytes(meta->mem_pool) > zram->limit_bytes) { >>>>> + total_bytes = zs_get_total_size_bytes(meta->mem_pool); >>>>> + if (zram->limit_bytes && total_bytes > zram->limit_bytes) { >>>> >>>> do you need to take the init_lock to read limit_bytes here? It could >>>> be getting changed between these checks... >>> >>> There is no real danger in freeing with an error. >>> It is more timing than a race. >> I probably should explain my reasoning. >> >> any changes between getting the total value and the limit test are not >> problematic (From race perspective). >> >> 1) If the actual total increases and the value returned under rates it, then >> a) if this.total exceeds the limit - no problem it is rolled back as >> it would if the actual total were used. >> b) if this.total <= limit OK - as other process will be dinged (it >> will see its own allocation) >> >> 2) If the actual total decreases and the value returned overrates >> rates it, then >> a) if this.value <= limit then allocation great (actual has even more room) >> b) if this.value > max it will be rolled back (as the other might be >> as well) and process can compete again. > for completeness I should have mentioned the normal decrease case of deallocation and not roll back. (and of course it is also not a problem and does not race). Are these typical situations in documentation folder (I know the related memory barriers is) It would be so much better to say scenario 23 is a potential problem rather than rewriting the essays. > actually I wasn't thinking of total_bytes changing, i think it's ok to > check the total at that specific point in time, for the reasons you > point out above. > > I was thinking about zram->limit_bytes changing, especially if it's > possible to disable the limit (i.e. set it to 0), e.g.: > > assume currently total_bytes == 1G and limit_bytes == 2G, so there is > not currently any danger of going over the limit. Then: > > > thread 1 : if (zram->limit_bytes ...this is true > > thread 2 : zram->limit_bytes = limit; ...where limit == 0 > > thread 1 : && total_bytes > zram->limit_bytes) { ...this is now also true > > thread 1 : incorrectly return -ENOMEM failure > > It's very unlikely, and a single failure isn't a big deal here since > the caller must be prepared to handle a failure. And of course the > compiler might reorder those checks. And if it's not possible to > disable the limit by setting it to 0 (besides a complete reset of the > zram device, which wouldn't happen while this function is running), > then there's not an issue here (although, I think being able to > disable the limit without having to reset the zram device is useful). agreed on 7 of 8 assertions (not yet sure about reset not happening while function running). That issue then arises in [PATCH 2/2] zram: limit memory size for zram and as you mention reordering the zero check after the limit comparison in the if statement could be reordered by the compiler As I see it this is also a timing issue - as you explained, and not a race. Perhaps we name it scenario 24? And especially I agree with allowing zero limit reset without device reset. The equivalent is currently possible (for all practical purposes) anyway by setting the limit to max_u64. So allowing zero is cleaner. > > > Also for setting the max_used_bytes, isn't non-atomically setting a > u64 value dangerous (on <64 bit systems) when it's not synchronized > between threads? perhaps it needs an atomic function - I will think some more on it. > > That is, unless the entire zram_bvec_write() function or this section > is already thread-safe, and i missed it (which i may have :-) nor have I.checked.(on the to do). > > >> -- 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/
Powered by blists - more mailing lists