[Discuss] smb: client: bitfield aliasing and locking chaos in cifsFileInfo

[Discuss] smb: client: bitfield aliasing and locking chaos in cifsFileInfo

[Frank Sorenson]

Hi all,

I ran xfstests on cifs/cifs-next (7.3-rc3+) with KCSAN enabled, and caught
a number of data races in the smb client.

The most concerning finding is a structural bitfield aliasing issue in
'struct cifsFileInfo'.

KCSAN caught a 1-byte write in '_cifsFileInfo_put' racing with a read in
'cifs_close':

   BUG: KCSAN: data-race in _cifsFileInfo_put [cifs] / cifs_close [cifs]

   write to 0xffff... of 1 bytes: _cifsFileInfo_put (file.c:885)
   read  to 0xffff... of 1 bytes: cifs_close (file.c:1495)

  885         cifs_file->offload = offload;
1495                 if ((cfile->status_file_deleted == false) &&


It also caught the same write racing with a read in 'cifs_prepare_write':

BUG: KCSAN: data-race in _cifsFileInfo_put [cifs] / cifs_prepare_write [cifs]

  write to 0xffff... of 1 bytes: _cifsFileInfo_put (file.c:885)
  read  to 0xffff... of 1 bytes: cifs_prepare_write (file.c:71)

  885         cifs_file->offload = offload;
   71         if (open_file->invalidHandle) {


In both cases, the read and write are to different fields of
'cifsFileInfo'.  However there are 5 consecutive 'bool:1' fields which all
share the same byte::

   bool invalidHandle:1;          /* bit 0 */
   bool swapfile:1;               /* bit 1 */
   bool oplock_break_cancelled:1; /* bit 2 */
   bool status_file_deleted:1;    /* bit 3 */
   bool offload:1;                /* bit 4 */


Since these bits all share the same byte, every write to any of these bits
becomes a byte-level read-modify-write cycle.


Looking at the codebase, the locking for these seems highly inconsistent.
For example, `invalidHandle` is sometimes written lockless, while
`oplock_break_cancelled` is written under either `file_info_lock` or
`tcon->open_file_lock`.  If any two of these write paths execute concurrently,
the byte-level RMW will silently discard one of the writes and corrupt the
bitfield.


It seems to me that we need to drop the ':1' from all 5 fields, eliminating the
byte-level aliasing entirely.

Beyond that, the access map (below) shows that the locking may need to be cleaned
up.  What is the intended locking for each of these fields?


Frank

Access map for the 5 bits across the client tree (excluding the single-threaded init writes):
   file:line              function                         R/W  lock held
   ---------              --------                         ---  ---------
invalidHandle:
   file.c:71              cifs_prepare_write               R    none
   file.c:128             cifs_issue_write                 R    none
   file.c:225             cifs_issue_read                  R    none
   file.c:397             cifs_mark_open_files_invalid     W=T  tcon->open_file_lock
   file.c:932             _cifsFileInfo_put                R    none (after unlock)
   file.c:1292            cifs_reopen_file                 R    fh_mutex
   file.c:1408            cifs_reopen_file                 W=F  fh_mutex
   file.c:1559            cifs_reopen_persistent_handles   R    tcon->open_file_lock
   file.c:1595            cifs_closedir                    W=T  file_info_lock
   file.c:2677            __find_readable_file             R    cifs_inode->open_file_lock
   file.c:2750            __cifs_get_writable_file         R    cifs_inode->open_file_lock
   readdir.c:45           dump_cifs_file_struct            R    none (debug)
   readdir.c:395          _initiate_cifs_search            W=T  none
   readdir.c:427          _initiate_cifs_search            W=F  none
   readdir.c:735          find_cifs_entry                  W=T  file_info_lock
   smb1ops.c:1283 (cb)    cifs_dir_needs_close             R    file_info_lock (caller)
   smb2ops.c:4606 (cb)    smb2_dir_needs_close             R    file_info_lock (caller)
oplock_break_cancelled:
   file.c:398             cifs_mark_open_files_invalid     W=T  tcon->open_file_lock
   file.c:3420            cifs_oplock_break                R    none
   smb1misc.c:162         is_valid_oplock_break            W=F  tcon->open_file_lock
   smb2misc.c:605         smb2_tcon_has_lease              W=F  tcon->open_file_lock
   smb2misc.c:607         smb2_tcon_has_lease              W=T  tcon->open_file_lock
   smb2misc.c:774         smb2_is_valid_oplock_break       W=T  file_info_lock
   smb2misc.c:776         smb2_is_valid_oplock_break       W=F  file_info_lock
offload:
   file.c:833             serverclose_work                 R    none
   file.c:885             _cifsFileInfo_put                W    tcon->open_file_lock +
                                                                 cifsi->open_file_lock +
                                                                 file_info_lock
status_file_deleted:
   file.c:1495            cifs_close                       R    none
   file.c:2670            __find_readable_file             R    cifs_inode->open_file_lock
   file.c:2743            __cifs_get_writable_file         R    cifs_inode->open_file_lock
   misc.c:675             cifs_mark_open_handles_for_deleted_file  W=T  cinode->open_file_lock
   misc.c:679             cifs_mark_open_handles_for_deleted_file  W=T  cinode->open_file_lock
swapfile:
   file.c:3510            cifs_swap_activate               W=T  none
   file.c:3528            cifs_swap_deactivate             W=F  none
   cifsfs.c:1648          cifs_copy_file_range             R    none


-- 
Frank Sorenson
sorenson@redhat.com
Principal Software Maintenance Engineer, filesystems
Red Hat

Re: [Discuss] smb: client: bitfield aliasing and locking chaos in cifsFileInfo

[Paulo Alcantara]

Frank Sorenson <sorenson@redhat.com> writes:

> I ran xfstests on cifs/cifs-next (7.3-rc3+) with KCSAN enabled, and caught
> a number of data races in the smb client.
>
> The most concerning finding is a structural bitfield aliasing issue in
> 'struct cifsFileInfo'.

Nice catch.

Please take a look at the following fix from Henrique

        ec306600d5ba ("smb: client: split cached_fid bitfields to avoid shared-byte RMW races")

You should probably fix them the same way.

Regarding the serialisation on them, yes, we would to check which one
really requires it.  That could be done separately, too.