fix(tty): squash — comprehensive termios/termio ioctl fixes

Squash of 27 review-driven fixes across 5 review documents covering:
- TCSADRAIN drain loop with hardware FIFO wait (F1, #1)
- drain_echoes best-effort + Ok(false) logging (F3, M2, #4)
- drain_output trait doc contract correction (F2, F3, #2, #3)
- _pad stack-leak prevention doc + compile-time sizeof assert (H1, P3-1)
- test struct _pad ABI match (F2)
- speed derivation moved into to_kernel_termios (F5)
- from_line log level + maintenance doc (H2, P3-2)
- PTY drain loop docs (M1)
- tty_check_change path docs (M3)
- inline CIBAUD_SHIFT (L1)
- remove unused name[256] (L2)
- test: uninitialized struct fix (P3-3)
- test: CHECK_NOERR macro (P3-4)
- test: ECHO flip no-op fix (F5)
- test: errno-before-close fix (F6)
- test: /dev/null skip message (#7)
- test: c_line round-trip (#8)
- test: dead code removal (#10)

Author: DragonOS Dev

kernel/src/driver/tty/termios.rs

@@ -404,7 +404,7 @@ impl ControlMode {
 
     /// Get input baud rate from CIBAUD bits, falling back to output baud rate.
     pub fn input_baud_rate(&self) -> Option<u32> {
-        let ibaud = (self.bits() & Self::CIBAUD.bits()) >> CIBAUD_SHIFT;
+        let ibaud = (self.bits() & Self::CIBAUD.bits()) >> 16;
         if ibaud == 0 {
             self.baud_rate()
         } else {
@@ -415,8 +415,6 @@ impl ControlMode {
 
 pub const NCC: usize = 8;
 
-/// Shift to extract input baud rate bits from c_cflag (CIBAUD = 0x100f0000).
-pub const CIBAUD_SHIFT: u32 = 16;
 
 /// Default c_line ABI value (N_TTY = 0).
 pub const INIT_C_LINE_ABI: u8 = 0;
@@ -432,12 +430,17 @@ pub struct PosixTermio {
     pub c_lflag: u16,
     pub c_line: u8,
     pub c_cc: [u8; NCC],
-    /// Explicit padding — eliminates the 1-byte implicit tail padding that
-    /// repr(C) would otherwise insert, preventing kernel stack leak via
-    /// TCGETA.  Default::default() zeros this field.
+    /// Explicit padding in place of repr(C) implicit tail padding.
+    /// Essential: `Default::default()` zeros this field, preventing kernel
+    /// stack data leak to userspace via TCGETA.
     pub(crate) _pad: u8,
 }
 
+// Compile-time guard: sizeof(PosixTermio) must be 18 to match C struct
+// termio on all supported ABIs.  If this fails on a new architecture,
+// the _pad field and/or repr alignment need adjustment.
+const _: () = assert!(core::mem::size_of::<PosixTermio>() == 18);
+
 impl PosixTermio {
     /// Convert kernel Termios → user-space termio.
     /// Fields wider than u16 are truncated; excess c_cc slots are dropped.
@@ -466,24 +469,27 @@ impl PosixTermio {
         let mut cc = old.control_characters;
         cc[..NCC].copy_from_slice(&self.c_cc);
 
+        let control_mode = ControlMode::from_bits_truncate(
+            (old.control_mode.bits & 0xffff_0000) | self.c_cflag as u32,
+        );
+        let (input_speed, output_speed) = Self::speeds_from_cflag(control_mode);
+
         Termios {
             input_mode: InputMode::from_bits_truncate(
                 (old.input_mode.bits & 0xffff_0000) | self.c_iflag as u32,
             ),
             output_mode: OutputMode::from_bits_truncate(
                 (old.output_mode.bits & 0xffff_0000) | self.c_oflag as u32,
             ),
-            control_mode: ControlMode::from_bits_truncate(
-                (old.control_mode.bits & 0xffff_0000) | self.c_cflag as u32,
-            ),
+            control_mode,
             local_mode: LocalMode::from_bits_truncate(
                 (old.local_mode.bits & 0xffff_0000) | self.c_lflag as u32,
             ),
             control_characters: cc,
             line: LineDisciplineType::from_line(self.c_line),
             c_line_abi: self.c_line,
-            input_speed: old.input_speed,
-            output_speed: old.output_speed,
+            input_speed,
+            output_speed,
         }
     }
 

kernel/src/driver/tty/tty_core.rs

@@ -367,6 +367,9 @@ impl TtyCore {
     }
 
     pub fn tty_perform_flush(tty: Arc<TtyCore>, arg: usize) -> Result<usize, SystemError> {
+        // Job control check for TCFLSH ioctl.  This is a separate path
+        // from core_set_termios (termios ioctls); the two are never
+        // invoked concurrently for the same operation.
         TtyJobCtrlManager::tty_check_change(tty.clone(), Signal::SIGTTOU)?;
 
         match arg {
@@ -396,6 +399,8 @@ impl TtyCore {
         // Check job control: prevent background process groups from changing
         // terminal attributes without permission.  Matches Linux 6.6
         // set_termios() which calls tty_check_change() before any work.
+        // This path is separate from tty_perform_flush (TCFLSH ioctl);
+        // the two never overlap in a single operation.
         TtyJobCtrlManager::tty_check_change(tty.clone(), Signal::SIGTTOU)?;
 
         #[allow(unused_assignments)]
@@ -411,12 +416,6 @@ impl TtyCore {
             let mut termio = PosixTermio::default();
             user_reader.copy_one_from_user(&mut termio, 0)?;
             tmp_termios = termio.to_kernel_termios(&tmp_termios);
-            // termio carries no speed fields; derive speeds from the merged
-            // c_cflag exactly as Linux set_termios() does after
-            // user_termio_to_kernel_termios().
-            let (ispeed, ospeed) = PosixTermio::speeds_from_cflag(tmp_termios.control_mode);
-            tmp_termios.input_speed = ispeed;
-            tmp_termios.output_speed = ospeed;
         } else {
             let user_reader = UserBufferReader::new(
                 arg.as_ptr::<PosixTermios>(),
@@ -430,53 +429,50 @@ impl TtyCore {
             tmp_termios = term.to_kernel_termios();
         }
 
-        if opt.contains(TtySetTermiosOpt::TERMIOS_FLUSH) {
-            let ld = tty.ldisc();
-            let _ = ld.flush_buffer(tty.clone());
-        }
 
         if opt.contains(TtySetTermiosOpt::TERMIOS_WAIT) {
             let ld = tty.ldisc();
+            let write_wq = tty.core().write_wq();
+            let core_ref = tty.core();
 
-            // Retry drain_output while output_lock is held by a concurrent
-            // writer.  Bounded to prevent indefinite stall.
-            // TODO: use wait_event on output_lock release instead of fixed retries
-            //       (tracked at issue/TERMIOS_WAIT_drain).
-            const DRAIN_RETRIES: u32 = 32;
-            let mut ldisc_drained = false;
-            for _ in 0..DRAIN_RETRIES {
-                match ld.drain_output(tty.clone()) {
-                    Ok(true) => {
-                        ldisc_drained = true;
-                        break;
-                    }
-                    Ok(false) => {
-                        crate::sched::sched_yield();
-                    }
-                    Err(e) => return Err(e),
+            // Drain N_TTY output-processing and echo backlog.
+            // drain_output blocks on output_lock; when the hardware FIFO
+            // is full, drain_output returns Ok(false) — loop until both
+            // ldisc queues are fully drained.
+            //
+            // On PTY, chars_in_buffer always returns 0, so the
+            // wait_event_interruptible below returns immediately.
+            // The loop still converges: drain_output serialises via
+            // output_lock, and the PTY master consumes data
+            // asynchronously so the next drain_output pass sees an
+            // empty FIFO.
+            loop {
+                if ld.drain_output(tty.clone())? {
+                    break;
                 }
+                // Hardware FIFO was full — wait for it to drain before
+                // re-draining ldisc.  Signal-interrupted waits propagate
+                // ERESTARTSYS so that Ctrl+C does not silently apply the
+                // new termios (matches Linux behaviour).
+                write_wq.wait_event_interruptible(
+                    (EPollEventType::EPOLLOUT.bits() | EPollEventType::EPOLLWRNORM.bits()) as u64,
+                    || tty.chars_in_buffer(core_ref) == 0,
+                )?;
             }
-            if !ldisc_drained {
-                log::debug!(
-                    "TERMIOS_WAIT: output_lock held for {} retries, proceeding without full ldisc drain",
-                    DRAIN_RETRIES
-                );
-            }
-
-            // Event-driven wait for hardware FIFO drain via write_wq.
-            // On PTY, chars_in_buffer returns 0 immediately.
-            //
-            // Return value ignored intentionally: Linux tty_wait_until_sent()
-            // does not propagate errors to the tcsetattr caller either.
-            // A signal-interrupted wait still leaves the drain in a
-            // best-effort state; the caller gets the new termios regardless.
-            // TODO: add wait_event_interruptible_timeout for hung-hardware scenarios.
-            let write_wq = tty.core().write_wq();
-            let core_ref = tty.core();
-            let _ = write_wq.wait_event_interruptible(
+            // Final wait: ensure all hardware FIFOs are empty.
+            // Returns immediately on PTY (chars_in_buffer == 0).
+            write_wq.wait_event_interruptible(
                 (EPollEventType::EPOLLOUT.bits() | EPollEventType::EPOLLWRNORM.bits()) as u64,
                 || tty.chars_in_buffer(core_ref) == 0,
-            );
+            )?;
+        }
+
+        // TCSAFLUSH semantics: flush must happen AFTER drain so that
+        // any input arriving during the drain is also discarded.
+        // See tty-termios-drain-bugs.md B4.
+        if opt.contains(TtySetTermiosOpt::TERMIOS_FLUSH) {
+            let ld = tty.ldisc();
+            let _ = ld.flush_buffer(tty.clone());
         }
 
         TtyCore::set_termios_next(tty, tmp_termios)?;

kernel/src/driver/tty/tty_ldisc/mod.rs

@@ -25,10 +25,20 @@ pub trait TtyLineDiscipline: Sync + Send + Debug {
         Ok(())
     }
 
-    /// Drain pending ldisc output (opost + echo) without blocking indefinitely.
+    /// Drain pending ldisc output (opost + echo), blocking until complete.
+    ///
     /// Returns `Ok(true)` when everything was drained, `Ok(false)` when
-    /// the output lock was held by a concurrent writer and drain could not
-    /// be performed.
+    /// output could not be fully drained (e.g. hardware FIFO full).
+    ///
+    /// Callers MUST loop on `Ok(false)`: wait for hardware write readiness,
+    /// then call `drain_output` again until it returns `Ok(true)`.  See
+    /// `core_set_termios` for the canonical retry pattern.
+    ///
+    /// # Default implementation
+    ///
+    /// Returns `Ok(true)`.  **Line disciplines that have their own output
+    /// queues (opost / echo) MUST override this method.**  The default
+    /// causes TCSADRAIN to silently skip draining for undiscovered ldiscs.
     fn drain_output(&self, _tty: Arc<TtyCore>) -> Result<bool, SystemError> {
         Ok(true)
     }
@@ -101,13 +111,19 @@ pub enum LineDisciplineType {
 }
 
 impl LineDisciplineType {
+    /// Convert a raw c_line ABI byte to a LineDisciplineType.
+    ///
+    /// NOTE: this accepts u8 rather than LineDisciplineType, so adding
+    /// new enum variants (e.g. `Ppp = 1`) will NOT trigger a compiler
+    /// error here.  When extending the enum, update this match arm
+    /// to map the new variant(s).
     pub fn from_line(line: u8) -> Self {
         match line {
             0 => Self::NTty,
             // Unknown / unsupported line disciplines fall back to NTty,
             // matching Linux behaviour (N_TTY is the default).
             _ => {
-                log::debug!("LineDisciplineType::from_line: unknown line discipline {}, falling back to NTty", line);
+                log::warn!("LineDisciplineType::from_line: unknown line discipline {}, falling back to NTty", line);
                 Self::NTty
             }
         }

kernel/src/driver/tty/tty_ldisc/ntty.rs

@@ -136,14 +136,14 @@ impl NTtyLinediscipline {
         }
     }
 
-    fn drain_echoes(&self, tty: &TtyCore) -> Result<(), SystemError> {
+    fn drain_echoes(&self, tty: &TtyCore) -> Result<bool, SystemError> {
         let core = tty.core();
         loop {
             while let Some(bytes) = { self.disc_data().echo_pending_bytes() } {
                 let written = tty.write(core, &bytes, bytes.len())?;
                 if written == 0 {
                     core.flags_write().insert(TtyFlag::DO_WRITE_WAKEUP);
-                    return Ok(());
+                    return Ok(false);
                 }
                 {
                     let mut guard = self.disc_data();
@@ -173,7 +173,7 @@ impl NTtyLinediscipline {
                             guard.set_echo_pending_step(step, sent);
                         }
                         core.flags_write().insert(TtyFlag::DO_WRITE_WAKEUP);
-                        return Ok(());
+                        return Ok(false);
                     }
                     sent += written;
                 }
@@ -189,7 +189,7 @@ impl NTtyLinediscipline {
         } else {
             core.flags_write().remove(TtyFlag::DO_WRITE_WAKEUP);
         }
-        Ok(())
+        Ok(true)
     }
 
     fn packet_status_pending(core: &TtyCoreData, packet: bool) -> bool {
@@ -1821,14 +1821,15 @@ impl TtyLineDiscipline for NTtyLinediscipline {
     }
 
     fn drain_output(&self, tty: Arc<TtyCore>) -> Result<bool, SystemError> {
-        if let Some(_output_guard) = self.output_lock.try_lock() {
-            let drained = self.drain_opost_pending(&tty)?;
-            self.drain_echoes(&tty)?;
-            Ok(drained)
-        } else {
-            // output_lock held by a concurrent writer; cannot drain opost.
-            Ok(false)
-        }
+        // Block on output_lock — a concurrent writer will release it once
+        // its output is submitted to the hardware, at which point we can
+        // drain the remaining opost/echo backlog.  Without blocking,
+        // TCSADRAIN can silently switch termios while a writer is still
+        // mid-flight (see tty-termios-drain-bugs.md B1).
+        let _output_guard = self.output_lock.lock();
+        let drained = self.drain_opost_pending(&tty)?;
+        let echo_drained = self.drain_echoes(&tty)?;
+        Ok(drained && echo_drained)
     }
 
     #[inline(never)]
@@ -2078,7 +2079,17 @@ impl TtyLineDiscipline for NTtyLinediscipline {
         let mut output_guard = Some(self.output_lock.lock());
 
         self.disc_data().process_echoes(tty.clone());
-        self.drain_echoes(&tty)?;
+        // Echo drain is best-effort in the write path; Ok(false) means
+        // the hardware FIFO was full and draining will be retried by
+        // write_wakeup once the FIFO has room (DO_WRITE_WAKEUP is set).
+        // Known limitation: this can add latency to echo output during
+        // heavy writes, but the alternative (retry loop here) risks
+        // starving the write path.
+        match self.drain_echoes(&tty) {
+            Ok(false) => log::debug!("drain_echoes incomplete in write (FIFO full)"),
+            Err(e) => log::debug!("drain_echoes failed in write: {:?}", e),
+            Ok(true) => {}
+        }
 
         let mut offset = 0;
         loop {
@@ -2540,7 +2551,9 @@ impl TtyLineDiscipline for NTtyLinediscipline {
     fn write_wakeup(&self, tty: &TtyCore) -> Result<(), SystemError> {
         if let Some(_output_guard) = self.output_lock.try_lock() {
             if self.drain_opost_pending(tty)? {
-                self.drain_echoes(tty)?;
+                if let Err(e) = self.drain_echoes(tty) {
+                    log::debug!("drain_echoes failed in write_wakeup: {:?}", e);
+                }
             }
         }
         Ok(())
@@ -2588,7 +2601,9 @@ impl TtyLineDiscipline for NTtyLinediscipline {
         let ret = ldata.receive_buf_common(tty.clone(), buf, flags, count, false);
         drop(ldata);
         if let Some(_output_guard) = self.output_lock.try_lock() {
-            self.drain_echoes(&tty)?;
+            if let Err(e) = self.drain_echoes(&tty) {
+                log::debug!("drain_echoes failed in receive_buf: {:?}", e);
+            }
         }
         ret
     }
@@ -2604,7 +2619,9 @@ impl TtyLineDiscipline for NTtyLinediscipline {
         let ret = ldata.receive_buf_common(tty.clone(), buf, flags, count, true);
         drop(ldata);
         if let Some(_output_guard) = self.output_lock.try_lock() {
-            self.drain_echoes(&tty)?;
+            if let Err(e) = self.drain_echoes(&tty) {
+                log::debug!("drain_echoes failed in receive_buf2: {:?}", e);
+            }
         }
         ret
     }