Skip to content
  • Vivek Goyal's avatar
    ovl: initialize OVL_UPPERDATA in ovl_lookup() · 28166ab3
    Vivek Goyal authored
    
    
    Currently ovl_get_inode() initializes OVL_UPPERDATA flag and for that it
    has to call ovl_check_metacopy_xattr() and check if metacopy xattr is
    present or not.
    
    yangerkun reported sometimes underlying filesystem might return -EIO and in
    that case error handling path does not cleanup properly leading to various
    warnings.
    
    Run generic/461 with ext4 upper/lower layer sometimes may trigger the bug
    as below(linux 4.19):
    
    [  551.001349] overlayfs: failed to get metacopy (-5)
    [  551.003464] overlayfs: failed to get inode (-5)
    [  551.004243] overlayfs: cleanup of 'd44/fd51' failed (-5)
    [  551.004941] overlayfs: failed to get origin (-5)
    [  551.005199] ------------[ cut here ]------------
    [  551.006697] WARNING: CPU: 3 PID: 24674 at fs/inode.c:1528 iput+0x33b/0x400
    ...
    [  551.027219] Call Trace:
    [  551.027623]  ovl_create_object+0x13f/0x170
    [  551.028268]  ovl_create+0x27/0x30
    [  551.028799]  path_openat+0x1a35/0x1ea0
    [  551.029377]  do_filp_open+0xad/0x160
    [  551.029944]  ? vfs_writev+0xe9/0x170
    [  551.030499]  ? page_counter_try_charge+0x77/0x120
    [  551.031245]  ? __alloc_fd+0x160/0x2a0
    [  551.031832]  ? do_sys_open+0x189/0x340
    [  551.032417]  ? get_unused_fd_flags+0x34/0x40
    [  551.033081]  do_sys_open+0x189/0x340
    [  551.033632]  __x64_sys_creat+0x24/0x30
    [  551.034219]  do_syscall_64+0xd5/0x430
    [  551.034800]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
    
    One solution is to improve error handling and call iget_failed() if error
    is encountered.  Amir thinks that this path is little intricate and there
    is not real need to check and initialize OVL_UPPERDATA in ovl_get_inode().
    Instead caller of ovl_get_inode() can initialize this state.  And this will
    avoid double checking of metacopy xattr lookup in ovl_lookup() and
    ovl_get_inode().
    
    OVL_UPPERDATA is inode flag.  So I was little concerned that initializing
    it outside ovl_get_inode() might have some races.  But this is one way
    transition.  That is once a file has been fully copied up, it can't go back
    to metacopy file again.  And that seems to help avoid races.  So as of now
    I can't see any races w.r.t OVL_UPPERDATA being set wrongly.  So move
    settingof OVL_UPPERDATA inside the callers of ovl_get_inode().
    ovl_obtain_alias() already does it.  So only two callers now left are
    ovl_lookup() and ovl_instantiate().
    
    Reported-by: default avataryangerkun <yangerkun@huawei.com>
    Suggested-by: default avatarAmir Goldstein <amir73il@gmail.com>
    Signed-off-by: default avatarVivek Goyal <vgoyal@redhat.com>
    Reviewed-by: default avatarAmir Goldstein <amir73il@gmail.com>
    Signed-off-by: default avatarMiklos Szeredi <mszeredi@redhat.com>
    28166ab3