On Thu, Aug 04, 2022 at 03:56:11AM +0100, Al Viro wrote: > Half a year too late, but then it hadn't been posted on fsdevel. > Which it really should have been, due to > > > + /* replace tcp socket to smc */ > > + smcsock->file = tcp->file; > > + smcsock->file->private_data = smcsock; > > + smcsock->file->f_inode = SOCK_INODE(smcsock); /* replace inode when sock_close */ > > + smcsock->file->f_path.dentry->d_inode = SOCK_INODE(smcsock); /* dput() in __fput */ > > + tcp->file = NULL; > > this. It violates a bunch of rather fundamental assertions about the > data structures you are playing with, and I'm not even going into the > lifetime and refcounting issues. > > * ->d_inode of a busy positive dentry never changes while refcount > of dentry remains positive. A lot of places in VFS rely upon that. > * ->f_inode of a file never changes, period. > * ->private_data of a struct file associated with a socket never > changes; it can be accessed lockless, with no precautions beyond "make sure > that refcount of struct file will remain positive". > > PS: more than one thread could be calling methods of that struct socket at the > same time; what's to stop e.g. connect(2) on the same sucker (e.g. called on > the same descriptor from a different thread that happens to share the same > descriptor table) to be sitting there trying to lock the struct sock currently > held locked by caller of tcp_set_ulp()? Sorry for the late reply. SMC ULP tries to make original TCP sockets behave like SMC. The original TCP sockets will belong to this new SMC socket, and it can only be accessed in kernel with struct socket in SMC. The SMC and TCP sockets are bonded together. So this patch replaces the file of TCP to SMC socket which is allocated in kernel. It is guaranteed that the TCP socket is always freed before the newly replaced SMC socket. There is an other approach to archive this by changing af_ops of sockets. I will fix it without breaking the assertions. Tony Lu
