Re: [PATCH net-next] net/smc: Introduce TCP ULP support

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

quoted

+	/* 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