Re: [f2fs-dev] [PATCH 1/2] fscrypt: allow synchronous bio decryption

[Jaegeuk Kim]

On 04/16, Eric Biggers wrote:
> Currently, fscrypt provides fscrypt_decrypt_bio_pages() which decrypts a
> bio's pages asynchronously, then unlocks them afterwards.  But, this
> assumes that decryption is the last "postprocessing step" for the bio,
> so it's incompatible with additional postprocessing steps such as
> authenticity verification after decryption.
> 
> Therefore, rename the existing fscrypt_decrypt_bio_pages() to
> fscrypt_enqueue_decrypt_bio().  Then, add fscrypt_decrypt_bio() which
> decrypts the pages in the bio synchronously without unlocking the pages,
> nor setting them Uptodate; and add fscrypt_enqueue_decrypt_work(), which
> enqueues work on the fscrypt_read_workqueue.  The new functions will be
> used by filesystems that support both fscrypt and fs-verity.

Hi Ted,

In order to prepare further work on fsverity, if you don't mind, can I queue
this patch in f2fs.git and upstream later?
Let me know, if you have any concern.

Thanks,

> 
> Signed-off-by: Eric Biggers 
> ---
>  fs/crypto/bio.c | 35 +
>  fs/crypto/crypto.c  |  8 +++-
>  fs/crypto/fscrypt_private.h |  1 -
>  fs/ext4/readpage.c  |  2 +-
>  fs/f2fs/data.c  |  2 +-
>  include/linux/fscrypt_notsupp.h | 13 +---
>  include/linux/fscrypt_supp.h|  5 -
>  7 files changed, 45 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c
> index 0d5e6a569d58..0959044c5cee 100644
> --- a/fs/crypto/bio.c
> +++ b/fs/crypto/bio.c
> @@ -26,15 +26,8 @@
>  #include 
>  #include "fscrypt_private.h"
>  
> -/*
> - * Call fscrypt_decrypt_page on every single page, reusing the encryption
> - * context.
> - */
> -static void completion_pages(struct work_struct *work)
> +static void __fscrypt_decrypt_bio(struct bio *bio, bool done)
>  {
> - struct fscrypt_ctx *ctx =
> - container_of(work, struct fscrypt_ctx, r.work);
> - struct bio *bio = ctx->r.bio;
>   struct bio_vec *bv;
>   int i;
>  
> @@ -46,22 +39,38 @@ static void completion_pages(struct work_struct *work)
>   if (ret) {
>   WARN_ON_ONCE(1);
>   SetPageError(page);
> - } else {
> + } else if (done) {
>   SetPageUptodate(page);
>   }
> - unlock_page(page);
> + if (done)
> + unlock_page(page);
>   }
> +}
> +
> +void fscrypt_decrypt_bio(struct bio *bio)
> +{
> + __fscrypt_decrypt_bio(bio, false);
> +}
> +EXPORT_SYMBOL(fscrypt_decrypt_bio);
> +
> +static void completion_pages(struct work_struct *work)
> +{
> + struct fscrypt_ctx *ctx =
> + container_of(work, struct fscrypt_ctx, r.work);
> + struct bio *bio = ctx->r.bio;
> +
> + __fscrypt_decrypt_bio(bio, true);
>   fscrypt_release_ctx(ctx);
>   bio_put(bio);
>  }
>  
> -void fscrypt_decrypt_bio_pages(struct fscrypt_ctx *ctx, struct bio *bio)
> +void fscrypt_enqueue_decrypt_bio(struct fscrypt_ctx *ctx, struct bio *bio)
>  {
>   INIT_WORK(>r.work, completion_pages);
>   ctx->r.bio = bio;
> - queue_work(fscrypt_read_workqueue, >r.work);
> + fscrypt_enqueue_decrypt_work(>r.work);
>  }
> -EXPORT_SYMBOL(fscrypt_decrypt_bio_pages);
> +EXPORT_SYMBOL(fscrypt_enqueue_decrypt_bio);
>  
>  void fscrypt_pullback_bio_page(struct page **page, bool restore)
>  {
> diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
> index ce654526c0fb..0758d32ad01b 100644
> --- a/fs/crypto/crypto.c
> +++ b/fs/crypto/crypto.c
> @@ -45,12 +45,18 @@ static mempool_t *fscrypt_bounce_page_pool = NULL;
>  static LIST_HEAD(fscrypt_free_ctxs);
>  static DEFINE_SPINLOCK(fscrypt_ctx_lock);
>  
> -struct workqueue_struct *fscrypt_read_workqueue;
> +static struct workqueue_struct *fscrypt_read_workqueue;
>  static DEFINE_MUTEX(fscrypt_init_mutex);
>  
>  static struct kmem_cache *fscrypt_ctx_cachep;
>  struct kmem_cache *fscrypt_info_cachep;
>  
> +void fscrypt_enqueue_decrypt_work(struct work_struct *work)
> +{
> + queue_work(fscrypt_read_workqueue, work);
> +}
> +EXPORT_SYMBOL(fscrypt_enqueue_decrypt_work);
> +
>  /**
>   * fscrypt_release_ctx() - Releases an encryption context
>   * @ctx: The encryption context to release.
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index ad6722bae8b7..4012558f6115 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -97,7 +97,6 @@ static inline bool fscrypt_valid_enc_modes(u32 
> contents_mode,
>  /* crypto.c */
>  extern struct kmem_cache *fscrypt_info_cachep;
>  extern int fscrypt_initialize(unsigned int cop_flags);
> -extern struct workqueue_struct *fscrypt_read_workqueue;
>  extern int fscrypt_do_page_crypto(const struct inode *inode,
> fscrypt_direction_t rw, u64 lblk_num,
> struct page *src_page,
> diff --git a/fs/ext4/readpage.c 

Re: [f2fs-dev] [PATCH] f2fs: set deadline to drop expired inmem pages

[Jaegeuk Kim]

On 04/17, Chao Yu wrote:
> On 2018/4/17 14:44, Chao Yu wrote:
> > On 2018/4/17 4:16, Jaegeuk Kim wrote:
> >> On 04/13, Chao Yu wrote:
> >>> On 2018/4/13 12:05, Jaegeuk Kim wrote:
>  On 04/13, Chao Yu wrote:
> > On 2018/4/13 9:04, Jaegeuk Kim wrote:
> >> On 04/10, Chao Yu wrote:
> >>> Hi Jaegeuk,
> >>>
> >>> On 2018/4/8 16:13, Chao Yu wrote:
>  f2fs doesn't allow abuse on atomic write class interface, so except
>  limiting in-mem pages' total memory usage capacity, we need to limit
>  start-commit time as well, otherwise we may run into infinite loop
>  during foreground GC because target blocks in victim segment are
>  belong to atomic opened file for long time.
> 
>  Now, we will check the condition with f2fs_balance_fs_bg in
>  background threads, once if user doesn't commit data exceeding 30
>  seconds, we will drop all cached data, so I expect it can keep our
>  system running safely to prevent Dos attack.
> >>>
> >>> Is it worth to add this patch to avoid abuse on atomic write 
> >>> interface by user?
> >>
> >> Hmm, hope to see a real problem first in this case.
> >
> > I think this can be a more critical security leak instead of a 
> > potential issue
> > which we can wait for someone reporting that can be too late.
> >
> > For example, user can simply write a huge file whose data spread in all 
> > f2fs
> > segments, once user open that file as atomic, foreground GC will suffer
> > deadloop, causing denying any further service of f2fs.
> 
>  How can you guarantee it won't happen within 30sec? If you want to avoid 
>  that,
> >>>
> >>> Now the value is smaller than generic hang task threshold in order to 
> >>> avoid
> >>> foreground GC helding gc_mutex too long, we can tune that parameter?
> >>>
>  you have to take a look at foreground gc.
> >>>
> >>> What do you mean? let GC moves blocks of atomic write opened file?
> >>
> >> I thought that we first need to detect when foreground GC is stuck by such 
> >> the
> >> huge number of atomic writes. Then, we need to do something like dropping 
> >> all
> >> the atomic writes.
> > 
> > Yup, that will be reasonable. :)
> 
> If we drop all atomic writes, for those atomic write who act very normal, it
> will case them losing all cached data without any hint like error return 
> value.
> So should we just:
> 
> - drop expired inmem pages.
> - or set FI_DROP_ATOMIC flag, return -EIO during atomic_commit, and reset the 
> flag.

Like FI_ATOMIC_REVOKE_REQUEST in atomic_commit?

> 
> Thanks,
> 
> > 
> > Thanks,
> > 
> >>
> >>>
> >>> Thanks,
> >>>
> 
> >
> > Thanks,
> >
> >>
> >>> Thanks,
> >>
> >> .
> >>
> 
>  .
> 
> >>
> >> .
> >>
> > 
> > 
> > .
> > 

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH v2] f2fs: allocate hot_data for atomic write more strictly

[Yunlei He]

If a file not set type as hot, has dirty pages more than
threshold 64 before starting atomic write, may be lose hot
flag.

v1->v2: move set FI_ATOMIC_FILE flag behind flush dirty pages too,
in case of dirty pages before starting atomic use atomic mode to
write back.

Signed-off-by: Yunlei He 
---
 fs/f2fs/file.c | 16 ++--
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 79eeed5..78b3a58 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1684,24 +1684,20 @@ static int f2fs_ioc_start_atomic_write(struct file 
*filp)
if (ret)
goto out;
 
-   set_inode_flag(inode, FI_ATOMIC_FILE);
-   set_inode_flag(inode, FI_HOT_DATA);
-   f2fs_update_time(F2FS_I_SB(inode), REQ_TIME);
-
if (!get_dirty_pages(inode))
-   goto inc_stat;
+   goto skip_flush;
 
f2fs_msg(F2FS_I_SB(inode)->sb, KERN_WARNING,
"Unexpected flush for atomic writes: ino=%lu, npages=%u",
inode->i_ino, get_dirty_pages(inode));
ret = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
-   if (ret) {
-   clear_inode_flag(inode, FI_ATOMIC_FILE);
-   clear_inode_flag(inode, FI_HOT_DATA);
+   if (ret)
goto out;
-   }
+skip_flush:
+   set_inode_flag(inode, FI_HOT_DATA);
+   set_inode_flag(inode, FI_ATOMIC_FILE);
+   f2fs_update_time(F2FS_I_SB(inode), REQ_TIME);
 
-inc_stat:
F2FS_I(inode)->inmem_task = current;
stat_inc_atomic_write(inode);
stat_update_max_atomic_write(inode);
-- 
1.9.1


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH v10 00/62] Convert page cache to XArray

[Goldwyn Rodrigues]

On 04/14/2018 02:58 PM, Matthew Wilcox wrote:
> On Sat, Apr 14, 2018 at 12:50:30PM -0700, Matthew Wilcox wrote:
>> On Mon, Apr 09, 2018 at 04:18:07PM -0500, Goldwyn Rodrigues wrote:
>>
>> I'm sorry I missed this email.  My inbox is a disaster :(
>>
>>> I tried these patches against next-20180329 and added the patch for the
>>> bug reported by Mike Kravetz. I am getting the following BUG on ext4 and
>>> xfs, running generic/048 tests of fstests. Each trace is from a
>>> different instance/run.
>>
>> Yikes.  I haven't been able to reproduce this.  Maybe it's a matter of
>> filesystem or some other quirk.
>>
>> It seems easy for you to reproduce it, so would you mind bisecting it?
>> Should be fairly straightforward; I'd start at commit "xarray: Add
>> MAINTAINERS entry", since the page cache shouldn't be affected by anything
>> up to that point, then bisect forwards from there.
>>
>>> BTW, for my convenience, do you have these patches in a public git tree?
>>
>> I didn't publish it; it's hard to push out a tree based on linux-next.
>> I'll try to make that happen.
> 
> Figured it out:
> 
> http://git.infradead.org/users/willy/linux-dax.git/shortlog/refs/heads/xarray-20180413
> 
> aka
>   git://git.infradead.org/users/willy/linux-dax.git xarray-20180413


Thanks.

I found the erroneous commit is
e14a33134244 mm: Convert workingset to XArray

mapping->nrexceptional is becoming negative.

An easy way to reproduce is to perform a large enough I/O to force it to
 swap out and inodes are evicted.

-- 
Goldwyn

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/2] f2fs: refactor read path to allow multiple postprocessing steps

[Eric Biggers via Linux-f2fs-devel]

Hi Chao,

On Tue, Apr 17, 2018 at 05:13:12PM +0800, Chao Yu wrote:
> > +
> > +static void bio_post_read_processing(struct bio_post_read_ctx *ctx);
> > +
> > +static void decrypt_work(struct work_struct *work)
> > +{
> > +   struct bio_post_read_ctx *ctx =
> > +   container_of(work, struct bio_post_read_ctx, work);
> > +
> > +   fscrypt_decrypt_bio(ctx->bio);
> > +
> > +   bio_post_read_processing(ctx);
> > +}
> > +
> > +static void bio_post_read_processing(struct bio_post_read_ctx *ctx)
> > +{
> > +   switch (++ctx->cur_step) {
> > +   case STEP_DECRYPT:
> > +   if (ctx->enabled_steps & (1 << STEP_DECRYPT)) {
> > +   INIT_WORK(>work, decrypt_work);
> > +   fscrypt_enqueue_decrypt_work(>work);
> > +   return;
> > +   }
> > +   ctx->cur_step++;
> > +   /* fall-through */
> > +   default:
> > +   __read_end_io(ctx->bio);
> > +   }
> 
> How about introducing __bio_post_read_processing()
> 
> switch (step) {
> case STEP_DECRYPT:
>   ...
>   break;
> case STEP_COMPRESS:
>   ...
>   break;
> case STEP_GENERIC:
>   __read_end_io;
>   break;
> ...
> }
> 
> Then we can customize flexible read processes like:
> 
> bio_post_read_processing()
> {
>   if (encrypt_enabled)
>   __bio_post_read_processing(, STEP_DECRYPT);
>   if (compress_enabled)
>   __bio_post_read_processing(, STEP_COMPRESS);
>   __bio_post_read_processing(, STEP_GENERIC);
> }
> 
> Or other flow.

If I understand correctly, you're suggesting that all the steps be done in a
single workqueue item?  The problem with that is that the verity work will
require I/O to the file to read hashes, which may need STEP_DECRYPT.  Hence,
decryption and verity will need separate workqueues.

> > @@ -481,29 +537,33 @@ static struct bio *f2fs_grab_read_bio(struct inode 
> > *inode, block_t blkaddr,
> >  unsigned nr_pages)
> >  {
> > struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > -   struct fscrypt_ctx *ctx = NULL;
> > struct bio *bio;
> > -
> > -   if (f2fs_encrypted_file(inode)) {
> > -   ctx = fscrypt_get_ctx(inode, GFP_NOFS);
> > -   if (IS_ERR(ctx))
> > -   return ERR_CAST(ctx);
> > -
> > -   /* wait the page to be moved by cleaning */
> > -   f2fs_wait_on_block_writeback(sbi, blkaddr);
> > -   }
> > +   struct bio_post_read_ctx *ctx;
> > +   unsigned int post_read_steps = 0;
> >  
> > bio = f2fs_bio_alloc(sbi, min_t(int, nr_pages, BIO_MAX_PAGES), false);
> > -   if (!bio) {
> > -   if (ctx)
> > -   fscrypt_release_ctx(ctx);
> > +   if (!bio)
> > return ERR_PTR(-ENOMEM);
> > -   }
> > f2fs_target_device(sbi, blkaddr, bio);
> > bio->bi_end_io = f2fs_read_end_io;
> > -   bio->bi_private = ctx;
> 
> bio->bi_private = NULL;
> 

I don't see why.  ->bi_private is NULL by default.

> > +   bio_post_read_ctx_pool =
> > +   mempool_create_slab_pool(128, bio_post_read_ctx_cache);
> 
> #define MAX_POST_READ_CACHE_SIZE  128
> 

Yes, that makes sense.

- Eric

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/2] f2fs: refactor read path to allow multiple postprocessing steps

[Eric Biggers via Linux-f2fs-devel]

Hi Michael,

On Mon, Apr 16, 2018 at 03:15:42PM -0700, Michael Halcrow wrote:
> Given recent talk I've seen on potentially applying file-based
> protections in NFS, I think it's worth making some cautionary
> observations at this stage.
> 
> Moxie's Cryptographic Doom Principle is an approachable take on the
> argument that one should verify before performing any other
> cryptographic operations.
> 
> https://moxie.org/blog/the-cryptographic-doom-principle/
> 
> As we consider inline cryptographic engines, this becomes challenging
> because encryption is delegated to the block layer, but authenticity
> still happens at the file system layer.
> 
> That said, the feasibility of applying attacks such as CCA2 against
> file sytem content is in question, depending on the adversarial model
> for the context in which the file system is integrated.  In
> particular, does the model include a MiTM between the kernel and the
> storage?
> 
> The fs-crypt adversarial model explicitly concerns itself only with a
> single point-in-time permanent offline compromise of storage.  I'm not
> aware of any real analysis anyone has done when we're instead dealing
> with storage that can be arbitrarily modified at arbitrary points in
> time.

I feel that your concerns are a bit off-topic for what this patch actually does,
but yes I don't think data confidentiality with fscrypt is guaranteed when an
attacker controls the disk.  And actually, fs-verity won't really change this
because fs-verity will only protect the file contents of immutable files.  It
won't protect filesystem metadata, or of mutable files.  In other words, most
files the user cares about the confidentiality of won't be protected by
fs-verity anyway.

Also, fs-verity will need to authenticate the plaintext (i.e. STEP_VERITY will
come after STEP_DECRYPT) because it may need to authenticate some well known
file, like an APK file that was signed by a certain entity.  But, when combined
with fscrypt, by the design of fscrypt on each device the file will be encrypted
with a unique key, resulting in a unique ciphertext.  The ciphertext is also not
visible to userspace currently so it would not be possible for a userspace tool
to generate the fs-verity metadata.

> 
> Is the user concerned about malicious drive controller firmware?  Will
> the content be transmitted over an insecure network connection?  Will
> the content be otherwise manipulatable by an adversary who can change
> to the backing storage?
> 
> The other caution relates to compress-then-encrypt.  I refer to the
> CRIME and BREACH attacks.
> 
> https://en.wikipedia.org/wiki/CRIME
> 
> https://en.wikipedia.org/wiki/BREACH
> 
> Again, it's context-dependent whether or how attacks like this can
> apply to a file system.  We'll need to be explicit about our
> expectations as we investigate whether or how to use fs-crypt and
> fs-verity capabilities in NFS.  I think this feeds into a discussion
> on efficient protection of file system metadata, but I'll defer to
> some future thread for that.

To be clear, neither f2fs nor NFS actually supports compression yet.  At least
for f2fs it's just a feature that some people want, and *might* be implemented
in the future.  Note that some people will want just compression and others will
want just encryption, so having both features available won't mean that everyone
will use both.  It will need to be discussed at that time how risky combining
them really is and whether it should be allowed or not -- or perhaps allowed but
with a warning explaining the risk.

> >  
> > -static void f2fs_read_end_io(struct bio *bio)
> > +/* postprocessing steps for read bios */
> > +enum bio_post_read_step {
> > +   STEP_INITIAL = 0,
> > +   STEP_DECRYPT,
> > +};
> > +
> > +struct bio_post_read_ctx {
> > +   struct bio *bio;
> > +   struct work_struct work;
> > +   unsigned int cur_step;
> > +   unsigned int enabled_steps;
> 
> What if enabled_steps has gaps?  E.g., 0x09 will be feasible if
> there's compression, encryption, virus signature scanning, and verity.
> I'm don't really see how the cur_step logic can be made to be elegant
> in that case.
> 
> If you start "inital" at 1, then you could collapse these two into
> "pending_steps", setting each bit position to 0 after processing.
> Just a thought.
> 

It will actually work fine.  In bio_post_read_processing(), we'll just fall
through and increment 'cur_step' until we get to the next step.  When there are
many steps we certainly could optimize it to use a bitmask and ffs() to
efficiently find the next step, but for now we are talking about maybe 1-3 steps
only, so I went with what seemed simplest.

Thanks,

Eric

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net

Re: [f2fs-dev] [PATCH 2/2] f2fs: support {d,id,did,x}node checksum

[Chao Yu]

On 2018/4/17 11:38, Jaegeuk Kim wrote:
> On 04/13, Chao Yu wrote:
>> Ping again..
>>
>> Do you have time to discuss this?
> 
> We may need a time to have a chat in person. Do you have any chance to visit
> US?

I prefer to, just count on LSF, but...

I think I need to find a conference which is opened in US first.

Just checked events.linuxfoundation.org, and didn't find any suitable conference
I could attend recently in US.

Location: US
Apr 18-20 Boston, Could Foundry Summit
Apr 23-25 Park City, LSF
Sep 24-26 Nashville, API strategy & practice
Oct 10-11 New York, Open FinTech Forum
Dec 11-13 Seattle, KubeCon & CloudNativeCon

Any other conferences?

Thanks,

> 
>>
>> On 2018/2/27 22:16, Chao Yu wrote:
>>> Ping,
>>>
>>> On 2018/2/13 15:34, Chao Yu wrote:
 Hi Jaegeuk,

 On 2018/2/10 10:52, Chao Yu wrote:
> On 2018/2/10 9:41, Jaegeuk Kim wrote:
>> On 02/01, Chao Yu wrote:
>>>
>>>
>>> On 2018/2/1 6:15, Jaegeuk Kim wrote:
 On 01/31, Chao Yu wrote:
> On 2018/1/31 10:02, Jaegeuk Kim wrote:
>> What if we want to add more entries in addition to node_checksum? Do 
>> we have
>> to add a new feature flag at every time? How about adding a layout 
>> value instead
>
> Hmm.. for previous implementation, IMO, we'd better add a new feature 
> flag at
> every time, otherwise, w/ extra_nsize only, in current image, we can 
> know a
> valid range of extended area in node block, but we don't know which
> fields/features are valid/enabled or not.
>
> One more thing is that if we can add one feature flag for each field, 
> we got one
> more chance to disable it dynamically.
>
>> of extra_nsize? For example, layout #1 means node_checksum with 
>> extra_nsize=X?
>>
>>
>> What does 1017 mean? We need to make this structure more flexibly 
>> for new
>
> Yes, using raw 1017 is not appropriate here.
>
>> entries. Like this?
>>  union {
>>  struct node_v1;
>>  struct node_v2;
>>  struct node_v3;
>>  ...
>>  struct direct_node dn;
>>  struct indirect_node in;
>>  };
>>  };
>>
>>  struct node_v1 {
>>  __le32 data[DEF_ADDRS_PER_BLOCK - V1_NSIZE=1];
>>  __le32 node_checksum;
>>  }
>>
>>  struct node_v2 {
>>  __le32 data[DEF_ADDRS_PER_BLOCK - V2_NSIZE=500];
>
> Hmm.. If we only need to add one more 4 bytes field in struct 
> node_v2, but
> V2_NSIZE is defined as fixed 500, there must be 492 bytes wasted.
>
> Or we can define V2_NSIZE as 8, but if there comes more and more 
> extended
> fields, node version count can be a large number, it results in 
> complicated
> version recognization and handling.
>
> One more question is how can we control which fields are valid or not 
> in
> comp[Vx_NSIZE]?
>
>
> Anyway, what I'm thinking is maybe we can restructure layout of node 
> block like
> the one used by f2fs_inode:
>
> struct f2fs_node {
>   union {
>   struct f2fs_inode i;
>   union {
>   struct {
>   __le32 node_checksum;
>   __le32 feature_field_1;
>   __le32 feature_field_2;
>   
>   __le32 addr[];
>   
>   };
>   struct direct_node dn;
>   struct indirect_node in;
>   };
>   };
>   struct node_footer footer;
> } __packed;
>
> Moving all extended fields to the head of f2fs_node, so we don't have 
> to use
> macro to indicate actual size of addr.

 Thinking what'd be the best way. My concern is, once getting more 
 entries, we
>>>
>>> OK, I think we need more discussion.. ;)
>>>
 can't set each of features individually. Like the second entry should 
 have
>>>
>>> Oh, that will be hard. If we have to avoid that, we have to tag in 
>>> somewhere
>>> e.g. f2fs_inode::i_flags2 to indicate which new field in f2fs_node is 
>>> valid, for
>>> example:
>>>
>>> #define F2FS_NODE_CHECKSUM  0x0001
>>> #define F2FS_NODE_FIELD10x0002
>>> #define 

Re: [f2fs-dev] [PATCH] f2fs: set deadline to drop expired inmem pages

[Chao Yu]

On 2018/4/17 14:44, Chao Yu wrote:
> On 2018/4/17 4:16, Jaegeuk Kim wrote:
>> On 04/13, Chao Yu wrote:
>>> On 2018/4/13 12:05, Jaegeuk Kim wrote:
 On 04/13, Chao Yu wrote:
> On 2018/4/13 9:04, Jaegeuk Kim wrote:
>> On 04/10, Chao Yu wrote:
>>> Hi Jaegeuk,
>>>
>>> On 2018/4/8 16:13, Chao Yu wrote:
 f2fs doesn't allow abuse on atomic write class interface, so except
 limiting in-mem pages' total memory usage capacity, we need to limit
 start-commit time as well, otherwise we may run into infinite loop
 during foreground GC because target blocks in victim segment are
 belong to atomic opened file for long time.

 Now, we will check the condition with f2fs_balance_fs_bg in
 background threads, once if user doesn't commit data exceeding 30
 seconds, we will drop all cached data, so I expect it can keep our
 system running safely to prevent Dos attack.
>>>
>>> Is it worth to add this patch to avoid abuse on atomic write interface 
>>> by user?
>>
>> Hmm, hope to see a real problem first in this case.
>
> I think this can be a more critical security leak instead of a potential 
> issue
> which we can wait for someone reporting that can be too late.
>
> For example, user can simply write a huge file whose data spread in all 
> f2fs
> segments, once user open that file as atomic, foreground GC will suffer
> deadloop, causing denying any further service of f2fs.

 How can you guarantee it won't happen within 30sec? If you want to avoid 
 that,
>>>
>>> Now the value is smaller than generic hang task threshold in order to avoid
>>> foreground GC helding gc_mutex too long, we can tune that parameter?
>>>
 you have to take a look at foreground gc.
>>>
>>> What do you mean? let GC moves blocks of atomic write opened file?
>>
>> I thought that we first need to detect when foreground GC is stuck by such 
>> the
>> huge number of atomic writes. Then, we need to do something like dropping all
>> the atomic writes.
> 
> Yup, that will be reasonable. :)

If we drop all atomic writes, for those atomic write who act very normal, it
will case them losing all cached data without any hint like error return value.
So should we just:

- drop expired inmem pages.
- or set FI_DROP_ATOMIC flag, return -EIO during atomic_commit, and reset the 
flag.

Thanks,

> 
> Thanks,
> 
>>
>>>
>>> Thanks,
>>>

>
> Thanks,
>
>>
>>> Thanks,
>>
>> .
>>

 .

>>
>> .
>>
> 
> 
> .
> 


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: check if inmem_pages list is empty correctly

[Chao Yu]

On 2018/4/17 17:12, Sheng Yong wrote:
> `cur' will never be NULL, we should check inmem_pages list instead.
> 
> Signed-off-by: Sheng Yong 

Reviewed-by: Chao Yu 

Thanks,

> ---
>  fs/f2fs/segment.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 5854cc4e1d67..bf9dab55b370 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -328,7 +328,7 @@ void drop_inmem_page(struct inode *inode, struct page 
> *page)
>   break;
>   }
>  
> - f2fs_bug_on(sbi, !cur || cur->page != page);
> + f2fs_bug_on(sbi, list_empty(head) || cur->page != page);
>   list_del(>list);
>   mutex_unlock(>inmem_lock);
>  
> 


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: allocate hot_data for atomic write more strictly

[Chao Yu]

On 2018/4/17 17:14, heyunlei wrote:
> 
> 
>> -Original Message-
>> From: Yuchao (T)
>> Sent: Tuesday, April 17, 2018 4:31 PM
>> To: heyunlei; jaeg...@kernel.org; linux-f2fs-devel@lists.sourceforge.net
>> Cc: Wangbintian; Zhangdianfang (Euler)
>> Subject: Re: [f2fs-dev][PATCH] f2fs: allocate hot_data for atomic write more 
>> strictly
>>
>> On 2018/4/16 19:34, Yunlei He wrote:
>>> If a file not set type as hot, has dirty pages more than
>>> threshold 64 before starting atomic write, may be lose hot
>>> flag.
>>>
>>> Signed-off-by: Yunlei He 
>>> ---
>>>  fs/f2fs/file.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>> index 79eeed5..bf61e20 100644
>>> --- a/fs/f2fs/file.c
>>> +++ b/fs/f2fs/file.c
>>> @@ -1685,7 +1685,6 @@ static int f2fs_ioc_start_atomic_write(struct file 
>>> *filp)
>>> goto out;
>>>
>>> set_inode_flag(inode, FI_ATOMIC_FILE);
>>
>> How about moving this below inc_stat tag? If there is still dirty page, for
>> reclaim path, we may redirty page with atomic write mode, we need to avoid 
>> that.
>>
> With it，maybe still has dirty pages after filemap_write_and_wait_range，
> which are not inmem pages and maybe affect atomicity?

I just send a patch, could you check that?

Thanks,

> 
> Thanks.
> 
>>> -   set_inode_flag(inode, FI_HOT_DATA);
>>> f2fs_update_time(F2FS_I_SB(inode), REQ_TIME);
>>
>> Ditto.
>>
>> Thanks,
>>
>>>
>>> if (!get_dirty_pages(inode))
>>> @@ -1697,11 +1696,11 @@ static int f2fs_ioc_start_atomic_write(struct file 
>>> *filp)
>>> ret = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
>>> if (ret) {
>>> clear_inode_flag(inode, FI_ATOMIC_FILE);
>>> -   clear_inode_flag(inode, FI_HOT_DATA);
>>> goto out;
>>> }
>>>
>>>  inc_stat:
>>> +   set_inode_flag(inode, FI_HOT_DATA);
>>> F2FS_I(inode)->inmem_task = current;
>>> stat_inc_atomic_write(inode);
>>> stat_update_max_atomic_write(inode);
>>>
> 


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH] f2fs: fix race in between GC and atomic open

[Chao Yu]

Thread  GC thread
- f2fs_ioc_start_atomic_write
 - get_dirty_pages
 - filemap_write_and_wait_range
- f2fs_gc
 - do_garbage_collect
  - gc_data_segment
   - move_data_page
- f2fs_is_atomic_file
- set_page_dirty
 - set_inode_flag(, FI_ATOMIC_FILE)

Dirty data page can still be generated by GC in race condition as
above call stack.

This patch adds fi->dio_rwsem[WRITE] in f2fs_ioc_start_atomic_write
to avoid such race.

Signed-off-by: Chao Yu 
---
 fs/f2fs/file.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 9ac837bb0efb..1612e763a69c 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1703,6 +1703,8 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
 
inode_lock(inode);
 
+   down_write(_I(inode)->dio_rwsem[WRITE]);
+
if (f2fs_is_atomic_file(inode))
goto out;
 
@@ -1728,6 +1730,7 @@ static int f2fs_ioc_start_atomic_write(struct file *filp)
stat_inc_atomic_write(inode);
stat_update_max_atomic_write(inode);
 out:
+   up_write(_I(inode)->dio_rwsem[WRITE]);
inode_unlock(inode);
mnt_drop_write_file(filp);
return ret;
-- 
2.15.0.55.gc2ece9dc4de6


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH] f2fs: check if inmem_pages list is empty correctly

[Sheng Yong]

`cur' will never be NULL, we should check inmem_pages list instead.

Signed-off-by: Sheng Yong 
---
 fs/f2fs/segment.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 5854cc4e1d67..bf9dab55b370 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -328,7 +328,7 @@ void drop_inmem_page(struct inode *inode, struct page *page)
break;
}
 
-   f2fs_bug_on(sbi, !cur || cur->page != page);
+   f2fs_bug_on(sbi, list_empty(head) || cur->page != page);
list_del(>list);
mutex_unlock(>inmem_lock);
 
-- 
2.14.1


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: allocate hot_data for atomic write more strictly

[heyunlei]

>-Original Message-
>From: Yuchao (T)
>Sent: Tuesday, April 17, 2018 4:31 PM
>To: heyunlei; jaeg...@kernel.org; linux-f2fs-devel@lists.sourceforge.net
>Cc: Wangbintian; Zhangdianfang (Euler)
>Subject: Re: [f2fs-dev][PATCH] f2fs: allocate hot_data for atomic write more 
>strictly
>
>On 2018/4/16 19:34, Yunlei He wrote:
>> If a file not set type as hot, has dirty pages more than
>> threshold 64 before starting atomic write, may be lose hot
>> flag.
>>
>> Signed-off-by: Yunlei He 
>> ---
>>  fs/f2fs/file.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 79eeed5..bf61e20 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -1685,7 +1685,6 @@ static int f2fs_ioc_start_atomic_write(struct file 
>> *filp)
>>  goto out;
>>
>>  set_inode_flag(inode, FI_ATOMIC_FILE);
>
>How about moving this below inc_stat tag? If there is still dirty page, for
>reclaim path, we may redirty page with atomic write mode, we need to avoid 
>that.
>
With it，maybe still has dirty pages after filemap_write_and_wait_range，
which are not inmem pages and maybe affect atomicity?

Thanks.

>> -set_inode_flag(inode, FI_HOT_DATA);
>>  f2fs_update_time(F2FS_I_SB(inode), REQ_TIME);
>
>Ditto.
>
>Thanks,
>
>>
>>  if (!get_dirty_pages(inode))
>> @@ -1697,11 +1696,11 @@ static int f2fs_ioc_start_atomic_write(struct file 
>> *filp)
>>  ret = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
>>  if (ret) {
>>  clear_inode_flag(inode, FI_ATOMIC_FILE);
>> -clear_inode_flag(inode, FI_HOT_DATA);
>>  goto out;
>>  }
>>
>>  inc_stat:
>> +set_inode_flag(inode, FI_HOT_DATA);
>>  F2FS_I(inode)->inmem_task = current;
>>  stat_inc_atomic_write(inode);
>>  stat_update_max_atomic_write(inode);
>>

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 2/2] f2fs: refactor read path to allow multiple postprocessing steps

[Chao Yu]

On 2018/4/17 3:31, Eric Biggers via Linux-f2fs-devel wrote:
> Currently f2fs's ->readpage() and ->readpages() assume that either the
> data undergoes no postprocessing, or decryption only.  But with
> fs-verity, there will be an additional authenticity verification step,
> and it may be needed either by itself, or combined with decryption.
> 
> To support this, store a 'struct bio_post_read_ctx' in ->bi_private
> which contains a work struct, a bitmask of postprocessing steps that are
> enabled, and an indicator of the current step.  The bio completion
> routine, if there was no I/O error, enqueues the first postprocessing
> step.  When that completes, it continues to the next step.  Pages that
> fail any postprocessing step have PageError set.  Once all steps have
> completed, pages without PageError set are set Uptodate, and all pages
> are unlocked.
> 
> Also replace f2fs_encrypted_file() with a new function
> f2fs_post_read_required() in places like direct I/O and garbage
> collection that really should be testing whether the file needs special
> I/O processing, not whether it is encrypted specifically.
> 
> This may also be useful for other future f2fs features such as
> compression.
> 
> Signed-off-by: Eric Biggers 
> ---
>  fs/f2fs/data.c   | 163 +++
>  fs/f2fs/f2fs.h   |  12 +++-
>  fs/f2fs/file.c   |   4 +-
>  fs/f2fs/gc.c |   6 +-
>  fs/f2fs/inline.c |   2 +-
>  fs/f2fs/super.c  |   6 ++
>  6 files changed, 144 insertions(+), 49 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 39225519de1c..ec70de0cd1d5 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -30,6 +30,9 @@
>  #include "trace.h"
>  #include 
>  
> +static struct kmem_cache *bio_post_read_ctx_cache;
> +static mempool_t *bio_post_read_ctx_pool;
> +
>  static bool __is_cp_guaranteed(struct page *page)
>  {
>   struct address_space *mapping = page->mapping;
> @@ -50,11 +53,77 @@ static bool __is_cp_guaranteed(struct page *page)
>   return false;
>  }
>  
> -static void f2fs_read_end_io(struct bio *bio)
> +/* postprocessing steps for read bios */
> +enum bio_post_read_step {
> + STEP_INITIAL = 0,
> + STEP_DECRYPT,
> +};
> +
> +struct bio_post_read_ctx {
> + struct bio *bio;
> + struct work_struct work;
> + unsigned int cur_step;
> + unsigned int enabled_steps;
> +};
> +
> +static void __read_end_io(struct bio *bio)
>  {
> - struct bio_vec *bvec;
> + struct page *page;
> + struct bio_vec *bv;
>   int i;
>  
> + bio_for_each_segment_all(bv, bio, i) {
> + page = bv->bv_page;
> +
> + /* PG_error was set if any post_read step failed */
> + if (bio->bi_status || PageError(page)) {
> + ClearPageUptodate(page);
> + SetPageError(page);
> + } else {
> + SetPageUptodate(page);
> + }
> + unlock_page(page);
> + }
> + if (bio->bi_private)
> + mempool_free(bio->bi_private, bio_post_read_ctx_pool);
> + bio_put(bio);
> +}
> +
> +static void bio_post_read_processing(struct bio_post_read_ctx *ctx);
> +
> +static void decrypt_work(struct work_struct *work)
> +{
> + struct bio_post_read_ctx *ctx =
> + container_of(work, struct bio_post_read_ctx, work);
> +
> + fscrypt_decrypt_bio(ctx->bio);
> +
> + bio_post_read_processing(ctx);
> +}
> +
> +static void bio_post_read_processing(struct bio_post_read_ctx *ctx)
> +{
> + switch (++ctx->cur_step) {
> + case STEP_DECRYPT:
> + if (ctx->enabled_steps & (1 << STEP_DECRYPT)) {
> + INIT_WORK(>work, decrypt_work);
> + fscrypt_enqueue_decrypt_work(>work);
> + return;
> + }
> + ctx->cur_step++;
> + /* fall-through */
> + default:
> + __read_end_io(ctx->bio);
> + }

How about introducing __bio_post_read_processing()

switch (step) {
case STEP_DECRYPT:
...
break;
case STEP_COMPRESS:
...
break;
case STEP_GENERIC:
__read_end_io;
break;
...
}

Then we can customize flexible read processes like:

bio_post_read_processing()
{
if (encrypt_enabled)
__bio_post_read_processing(, STEP_DECRYPT);
if (compress_enabled)
__bio_post_read_processing(, STEP_COMPRESS);
__bio_post_read_processing(, STEP_GENERIC);
}

Or other flow.

> +}
> +
> +static bool f2fs_bio_post_read_required(struct bio *bio)
> +{
> + return bio->bi_private && !bio->bi_status;
> +}
> +
> +static void f2fs_read_end_io(struct bio *bio)
> +{
>  #ifdef CONFIG_F2FS_FAULT_INJECTION
>   if (time_to_inject(F2FS_P_SB(bio_first_page_all(bio)), FAULT_IO)) {
>   f2fs_show_injection_info(FAULT_IO);
> @@ -62,28 +131,15 @@ static void f2fs_read_end_io(struct bio *bio)
>   }
>  #endif
>  
> - if 

Re: [f2fs-dev] [PATCH] f2fs: allocate hot_data for atomic write more strictly

[Chao Yu]

On 2018/4/16 19:34, Yunlei He wrote:
> If a file not set type as hot, has dirty pages more than
> threshold 64 before starting atomic write, may be lose hot
> flag.
> 
> Signed-off-by: Yunlei He 
> ---
>  fs/f2fs/file.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 79eeed5..bf61e20 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -1685,7 +1685,6 @@ static int f2fs_ioc_start_atomic_write(struct file 
> *filp)
>   goto out;
>  
>   set_inode_flag(inode, FI_ATOMIC_FILE);

How about moving this below inc_stat tag? If there is still dirty page, for
reclaim path, we may redirty page with atomic write mode, we need to avoid that.

> - set_inode_flag(inode, FI_HOT_DATA);
>   f2fs_update_time(F2FS_I_SB(inode), REQ_TIME);

Ditto.

Thanks,

>  
>   if (!get_dirty_pages(inode))
> @@ -1697,11 +1696,11 @@ static int f2fs_ioc_start_atomic_write(struct file 
> *filp)
>   ret = filemap_write_and_wait_range(inode->i_mapping, 0, LLONG_MAX);
>   if (ret) {
>   clear_inode_flag(inode, FI_ATOMIC_FILE);
> - clear_inode_flag(inode, FI_HOT_DATA);
>   goto out;
>   }
>  
>  inc_stat:
> + set_inode_flag(inode, FI_HOT_DATA);
>   F2FS_I(inode)->inmem_task = current;
>   stat_inc_atomic_write(inode);
>   stat_update_max_atomic_write(inode);
> 


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: Copy summary entries in bulk

[Chao Yu]

On 2018/4/16 19:48, LiFan wrote:
> Copy summary entries in bulk in write_compacted_summaries().
> And zero the blank part of page after writing the summaries
> to the page.
> 
> Signed-off-by: Fan li 
> ---
>  fs/f2fs/segment.c | 38 ++
>  1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 3bc77c2..a08b2a8 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -3103,14 +3103,12 @@ static void write_compacted_summaries(struct 
> f2fs_sb_info *sbi, block_t blkaddr)
>  {
>   struct page *page;
>   unsigned char *kaddr;
> - struct f2fs_summary *summary;
>   struct curseg_info *seg_i;
>   int written_size = 0;
> - int i, j;
> + int i;
>  
>   page = grab_meta_page(sbi, blkaddr++);
>   kaddr = (unsigned char *)page_address(page);
> - memset(kaddr, 0, PAGE_SIZE);
>  
>   /* Step 1: write nat cache */
>   seg_i = CURSEG_I(sbi, CURSEG_HOT_DATA);
> @@ -3124,34 +3122,42 @@ static void write_compacted_summaries(struct 
> f2fs_sb_info *sbi, block_t blkaddr)
>  
>   /* Step 3: write summary entries */
>   for (i = CURSEG_HOT_DATA; i <= CURSEG_COLD_DATA; i++) {
> - unsigned short blkoff;
> + unsigned short blkoff, page_cap, summary_cnt;

blkoff = 0;

> +
>   seg_i = CURSEG_I(sbi, i);
>   if (sbi->ckpt->alloc_type[i] == SSR)
>   blkoff = sbi->blocks_per_seg;

max = sbi->blocks_per_seg;

>   else
>   blkoff = curseg_blkoff(sbi, i);

max = curseg_blkoff(sbi, i);

>  
> - for (j = 0; j < blkoff; j++) {
> + while (blkoff) {

while (blkoff < max)

>   if (!page) {
>   page = grab_meta_page(sbi, blkaddr++);
>   kaddr = (unsigned char *)page_address(page);
> - memset(kaddr, 0, PAGE_SIZE);
>   written_size = 0;
>   }
> - summary = (struct f2fs_summary *)(kaddr + written_size);
> - *summary = seg_i->sum_blk->entries[j];
> - written_size += SUMMARY_SIZE;
>  
> - if (written_size + SUMMARY_SIZE <= PAGE_SIZE -
> - SUM_FOOTER_SIZE)
> - continue;
> -
> - set_page_dirty(page);
> - f2fs_put_page(page, 1);
> - page = NULL;
> + page_cap = (PAGE_SIZE - SUM_FOOTER_SIZE - written_size)
> + / SUMMARY_SIZE;
> + summary_cnt = min(page_cap, blkoff);

summary_cnt = min(page_cap, max - blkoff);

> + memcpy(kaddr + written_size, seg_i->sum_blk->entries,

memcpy(kaddr + written_size, seg_i->sum_blk->entries + blkoff

> + summary_cnt * SUMMARY_SIZE);

blkoff += summary_cnt;

> + written_size += summary_cnt * SUMMARY_SIZE;
> + blkoff -= summary_cnt;

removed.

Thanks,

> +
> + if (page_cap == summary_cnt) {
> + memset(kaddr + written_size, 0,
> + PAGE_SIZE - written_size);
> + set_page_dirty(page);
> + f2fs_put_page(page, 1);
> + page = NULL;
> + }
>   }
>   }
> +
>   if (page) {
> + memset(kaddr + written_size, 0,
> + PAGE_SIZE - written_size);
>   set_page_dirty(page);
>   f2fs_put_page(page, 1);
>   }
> 


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: set deadline to drop expired inmem pages

[Chao Yu]

On 2018/4/17 4:16, Jaegeuk Kim wrote:
> On 04/13, Chao Yu wrote:
>> On 2018/4/13 12:05, Jaegeuk Kim wrote:
>>> On 04/13, Chao Yu wrote:
 On 2018/4/13 9:04, Jaegeuk Kim wrote:
> On 04/10, Chao Yu wrote:
>> Hi Jaegeuk,
>>
>> On 2018/4/8 16:13, Chao Yu wrote:
>>> f2fs doesn't allow abuse on atomic write class interface, so except
>>> limiting in-mem pages' total memory usage capacity, we need to limit
>>> start-commit time as well, otherwise we may run into infinite loop
>>> during foreground GC because target blocks in victim segment are
>>> belong to atomic opened file for long time.
>>>
>>> Now, we will check the condition with f2fs_balance_fs_bg in
>>> background threads, once if user doesn't commit data exceeding 30
>>> seconds, we will drop all cached data, so I expect it can keep our
>>> system running safely to prevent Dos attack.
>>
>> Is it worth to add this patch to avoid abuse on atomic write interface 
>> by user?
>
> Hmm, hope to see a real problem first in this case.

 I think this can be a more critical security leak instead of a potential 
 issue
 which we can wait for someone reporting that can be too late.

 For example, user can simply write a huge file whose data spread in all 
 f2fs
 segments, once user open that file as atomic, foreground GC will suffer
 deadloop, causing denying any further service of f2fs.
>>>
>>> How can you guarantee it won't happen within 30sec? If you want to avoid 
>>> that,
>>
>> Now the value is smaller than generic hang task threshold in order to avoid
>> foreground GC helding gc_mutex too long, we can tune that parameter?
>>
>>> you have to take a look at foreground gc.
>>
>> What do you mean? let GC moves blocks of atomic write opened file?
> 
> I thought that we first need to detect when foreground GC is stuck by such the
> huge number of atomic writes. Then, we need to do something like dropping all
> the atomic writes.

Yup, that will be reasonable. :)

Thanks,

> 
>>
>> Thanks,
>>
>>>

 Thanks,

>
>> Thanks,
>
> .
>
>>>
>>> .
>>>
> 
> .
> 


--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel