Race: data race between shmem_getpage() and mapping_set_gfp_mask()

Race: data race between shmem_getpage() and mapping_set_gfp_mask()

[Gong, Sishuai]

Hi,

We found a data race in linux kernel 5.3.11 that we are able to reproduce in x86 under specific interleavings. Currently, we are not sure about the consequence of this race so we would like to confirm with the community if this can be a harmful bug.

------------------------------------------
Writer site

 /tmp/tmp.B7zb7od2zE-5.3.11/extract/linux-5.3.11/include/linux/pagemap.h:118
         98  }
         99
        100  static inline gfp_t mapping_gfp_mask(struct address_space * mapping)
        101  {
        102      return mapping->gfp_mask;
        103  }
        104
        105  /* Restricts the given gfp_mask to what the mapping allows. */
        106  static inline gfp_t mapping_gfp_constraint(struct address_space *mapping,
        107          gfp_t gfp_mask)
        108  {
        109      return mapping_gfp_mask(mapping) & gfp_mask;
        110  }
        111
        112  /*
        113   * This is non-atomic.  Only to be used before the mapping is activated.
        114   * Probably needs a barrier...
        115   */
        116  static inline void mapping_set_gfp_mask(struct address_space *m, gfp_t mask)
        117  {
 ==>    118      m->gfp_mask = mask;
        119  }

------------------------------------------
Reader site

/tmp/tmp.B7zb7od2zE-5.3.11/extract/linux-5.3.11/mm/shmem.c:139
        119
        120      return min(nr_pages - totalhigh_pages(), nr_pages / 2);
        121  }
        122  #endif
        123
        124  static bool shmem_should_replace_page(struct page *page, gfp_t gfp);
        125  static int shmem_replace_page(struct page **pagep, gfp_t gfp,
        126                  struct shmem_inode_info *info, pgoff_t index);
        127  static int shmem_swapin_page(struct inode *inode, pgoff_t index,
        128                   struct page **pagep, enum sgp_type sgp,
        129                   gfp_t gfp, struct vm_area_struct *vma,
        130                   vm_fault_t *fault_type);
        131  static int shmem_getpage_gfp(struct inode *inode, pgoff_t index,
        132          struct page **pagep, enum sgp_type sgp,
        133          gfp_t gfp, struct vm_area_struct *vma,
        134          struct vm_fault *vmf, vm_fault_t *fault_type);
        135
        136  int shmem_getpage(struct inode *inode, pgoff_t index,
        137          struct page **pagep, enum sgp_type sgp)
        138  {
 ==>    139      return shmem_getpage_gfp(inode, index, pagep, sgp,
        140          mapping_gfp_mask(inode->i_mapping), NULL, NULL, NULL);
        141  }
        

------------------------------------------
Writer calling trace

- ksys_ioctl
-- do_vfs_ioctl
--- vfs_ioctl
---- blkdev_ioctl
----- __blkdev_driver_ioctl
------ loop_set_fd

------------------------------------------
Reader calling trace

- ksys_read
-- vfs_read
--- __vfs_read
---- shmem_getpage



Thanks,
Sishuai

Re: Race: data race between shmem_getpage() and mapping_set_gfp_mask()

[Matthew Wilcox]

On Mon, Nov 30, 2020 at 06:14:29PM +0000, Gong, Sishuai wrote:
> Hi,
> 
> We found a data race in linux kernel 5.3.11 that we are able to reproduce in x86 under specific interleavings. Currently, we are not sure about the consequence of this race so we would like to confirm with the community if this can be a harmful bug.

Are you going to be sending a lot of these?  There's no shortage of
actual bugs hit with syzcaller.  What we're short on is people to review,
analyse and fix bugs.

Re: Race: data race between shmem_getpage() and mapping_set_gfp_mask()

[Gong, Sishuai]

Thanks for your feedback. We are sorry that we didn’t send detailed reports as expected and the trouble we caused. We will try to provide more analysis for the ones we reported and to be reported in the next.

Our analysis shows this data race didn’t have explicit harmful impact in this case and we want to report our experience with the experiment.

------------------------------------------
Interleaving
We observed two interleavings of the two racy instructions. As shown below, the reader can see an inconsistent value but it is not used in both interleavings.
We are wondering if the xa_is_value() condition could be true. Possibly the inconsistent value will be passed to shmem_swapin_page() and cause some error.

Interleaving 1
Writer							Reader
m->gfp_mask = mask;
// write value = 0x100c0a
								return shmem_getpage_gfp(inode, index, pagep, sgp, mapping_gfp_mask(inode->i_mapping), NULL, NULL, NULL);
								// read value = 0x100c0a
								…
								// in function shmem_getpage_gfp()
								if (xa_is_value(page)) {
								// condition is false, skip
									error = shmem_swapin_page(inode, index, &page, sgp, gfp, vma, fault_type);


Interleaving 2
Writer							Reader
								return shmem_getpage_gfp(inode, index, pagep, sgp, mapping_gfp_mask(inode->i_mapping), NULL, NULL, NULL);
								// read value = 0x100caa

m->gfp_mask = mask;
// write value = 0x100c0a
								…
								// in function shmem_getpage_gfp()
								if (xa_is_value(page)) {
								// condition is false, skip
								       error = shmem_swapin_page(inode, index, &page, sgp, gfp, vma, fault_type);


Thanks,
Sishuai

> On Nov 30, 2020, at 1:16 PM, Matthew Wilcox <willy@infradead.org> wrote:
> 
> On Mon, Nov 30, 2020 at 06:14:29PM +0000, Gong, Sishuai wrote:
>> Hi,
>> 
>> We found a data race in linux kernel 5.3.11 that we are able to reproduce in x86 under specific interleavings. Currently, we are not sure about the consequence of this race so we would like to confirm with the community if this can be a harmful bug.
> 
> Are you going to be sending a lot of these?  There's no shortage of
> actual bugs hit with syzcaller.  What we're short on is people to review,
> analyse and fix bugs.